__link_block_group uses GFP_KERNEL

__link_block_group uses GFP_KERNEL

[Denis Kirjanov]

Hi guys,

Looks like that current code does GFP_KERNEL allocation inside
__link_block_group.
the function invokes kobject_add and internally creates sysfs files
with the GFP_KERNEL flag set.
But since do_chunk_alloc executes insides the btrfs transaction it's
not allowed to sleep.

Thanks!

Re: __link_block_group uses GFP_KERNEL

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 661 bytes --]

On 3/24/17 5:02 AM, Denis Kirjanov wrote:
> Hi guys,
> 
> Looks like that current code does GFP_KERNEL allocation inside
> __link_block_group.
> the function invokes kobject_add and internally creates sysfs files
> with the GFP_KERNEL flag set.

Yep, that's a bug.

> But since do_chunk_alloc executes insides the btrfs transaction it's
> not allowed to sleep.

It's allowed to sleep but isn't allowed to do reclaim that involves file
system writeback.  Michal Hocko's allocation context idea would fix
this, but it's not there yet, so we'll need to defer the kobject_add
until we can use GFP_KERNEL.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: __link_block_group uses GFP_KERNEL

[Denis Kirjanov]

On 3/25/17, Jeff Mahoney <jeffm@suse.com> wrote:
> On 3/24/17 5:02 AM, Denis Kirjanov wrote:
>> Hi guys,
>>
>> Looks like that current code does GFP_KERNEL allocation inside
>> __link_block_group.
>> the function invokes kobject_add and internally creates sysfs files
>> with the GFP_KERNEL flag set.
>
> Yep, that's a bug.
>
>> But since do_chunk_alloc executes insides the btrfs transaction it's
>> not allowed to sleep.
>
> It's allowed to sleep but isn't allowed to do reclaim that involves file
> system writeback.  Michal Hocko's allocation context idea would fix
> this, but it's not there yet, so we'll need to defer the kobject_add
> until we can use GFP_KERNEL.

Ok, I see. Can you point out to the initial patchset?

Thanks!

>
> -Jeff
>
> --
> Jeff Mahoney
> SUSE Labs
>
>

Re: __link_block_group uses GFP_KERNEL

[David Sterba]

On Sat, Mar 25, 2017 at 09:48:28AM +0300, Denis Kirjanov wrote:
> On 3/25/17, Jeff Mahoney <jeffm@suse.com> wrote:
> > On 3/24/17 5:02 AM, Denis Kirjanov wrote:
> >> Hi guys,
> >>
> >> Looks like that current code does GFP_KERNEL allocation inside
> >> __link_block_group.
> >> the function invokes kobject_add and internally creates sysfs files
> >> with the GFP_KERNEL flag set.
> >
> > Yep, that's a bug.
> >
> >> But since do_chunk_alloc executes insides the btrfs transaction it's
> >> not allowed to sleep.
> >
> > It's allowed to sleep but isn't allowed to do reclaim that involves file
> > system writeback.  Michal Hocko's allocation context idea would fix
> > this, but it's not there yet, so we'll need to defer the kobject_add
> > until we can use GFP_KERNEL.
> 
> Ok, I see. Can you point out to the initial patchset?

https://lwn.net/Articles/716323/

Fixing this properly is a lot of work so we might need to add a
temporary workaround, as Jeff suggests, to move calling into sysfs to a
later time.

Re: __link_block_group uses GFP_KERNEL

[Denis Kirjanov]

On 3/27/17, David Sterba <dsterba@suse.cz> wrote:
> On Sat, Mar 25, 2017 at 09:48:28AM +0300, Denis Kirjanov wrote:
>> On 3/25/17, Jeff Mahoney <jeffm@suse.com> wrote:
>> > On 3/24/17 5:02 AM, Denis Kirjanov wrote:
>> >> Hi guys,
>> >>
>> >> Looks like that current code does GFP_KERNEL allocation inside
>> >> __link_block_group.
>> >> the function invokes kobject_add and internally creates sysfs files
>> >> with the GFP_KERNEL flag set.
>> >
>> > Yep, that's a bug.
>> >
>> >> But since do_chunk_alloc executes insides the btrfs transaction it's
>> >> not allowed to sleep.
>> >
>> > It's allowed to sleep but isn't allowed to do reclaim that involves
>> > file
>> > system writeback.  Michal Hocko's allocation context idea would fix
>> > this, but it's not there yet, so we'll need to defer the kobject_add
>> > until we can use GFP_KERNEL.
>>
>> Ok, I see. Can you point out to the initial patchset?
>
> https://lwn.net/Articles/716323/
>
> Fixing this properly is a lot of work so we might need to add a
> temporary workaround, as Jeff suggests, to move calling into sysfs to a
> later time.
>

Care to send a patch?
Or I can dig a bit.

Thanks!