kdave/for-next commit 26112f7f472

kdave/for-next commit 26112f7f472

[Jeff Mahoney]

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

Hi Dave -

This commit introduces a bug.  I ran across it when running xfstests
against my own integrated branch.

The problem is that btrfs_calc_reclaim_metadata_size didn't used to be
called from recovery, so it was safe to use fs_info->fs_root.  With
commit 7c83c6a09 (Btrfs: don't bother kicking async if there's nothing
to reclaim) we do call it from recovery context and fs_info->fs_root is
NULL.

The fix is to just not switch btrfs_calc_reclaim_metadata_size to take
an fs_info.  All the other call sites were using fs_info->fs_root
anyway, so it's not like we're pinning a root somewhere just for this call.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


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

Re: kdave/for-next commit 26112f7f472

[Holger Hoffstätte]

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

On 07/08/16 06:24, Jeff Mahoney wrote:
> Hi Dave -
> 
> This commit introduces a bug.  I ran across it when running xfstests
> against my own integrated branch.

I can't find that commit id anywhere...?

> The problem is that btrfs_calc_reclaim_metadata_size didn't used to be
> called from recovery, so it was safe to use fs_info->fs_root.  With
> commit 7c83c6a09 (Btrfs: don't bother kicking async if there's nothing
> to reclaim) we do call it from recovery context and fs_info->fs_root is
> NULL.
> 
> The fix is to just not switch btrfs_calc_reclaim_metadata_size to take
> an fs_info.  All the other call sites were using fs_info->fs_root
> anyway, so it's not like we're pinning a root somewhere just for this call.

I've had this patch from last October in my 4.4.x tree forever:
http://www.spinics.net/lists/linux-btrfs/msg48457.html

Apparently it fell off the table. Shouldn't that fix it?

-h



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

Re: kdave/for-next commit 26112f7f472

[Jeff Mahoney]

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

On 7/8/16 7:19 AM, Holger Hoffstätte wrote:
> On 07/08/16 06:24, Jeff Mahoney wrote:
>> Hi Dave -
>>
>> This commit introduces a bug.  I ran across it when running xfstests
>> against my own integrated branch.
> 
> I can't find that commit id anywhere...?

Hi Holger -

This is the for-next branch.  It's not in any mainline branch yet.

>> The problem is that btrfs_calc_reclaim_metadata_size didn't used to be
>> called from recovery, so it was safe to use fs_info->fs_root.  With
>> commit 7c83c6a09 (Btrfs: don't bother kicking async if there's nothing
>> to reclaim) we do call it from recovery context and fs_info->fs_root is
>> NULL.
>>
>> The fix is to just not switch btrfs_calc_reclaim_metadata_size to take
>> an fs_info.  All the other call sites were using fs_info->fs_root
>> anyway, so it's not like we're pinning a root somewhere just for this call.
> 
> I've had this patch from last October in my 4.4.x tree forever:
> http://www.spinics.net/lists/linux-btrfs/msg48457.html
> 
> Apparently it fell off the table. Shouldn't that fix it?

A different fix went into for-next.  That's where the conflict is.  The
merged version of my root->fs_info patch reverts it.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


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

Re: kdave/for-next commit 26112f7f472

[Holger Hoffstätte]

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

On 07/08/16 13:55, Jeff Mahoney wrote:
> On 7/8/16 7:19 AM, Holger Hoffstätte wrote:
>> On 07/08/16 06:24, Jeff Mahoney wrote:
>>> Hi Dave -
>>>
>>> This commit introduces a bug.  I ran across it when running xfstests
>>> against my own integrated branch.
>>
>> I can't find that commit id anywhere...?
> 
> Hi Holger -
> 
> This is the for-next branch.  It's not in any mainline branch yet.

Yes, I understand that. I searched in the github/kdave tree, which only
has the for-next-xyz branches. Found it in the one on kernel.org.

-h



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

Re: kdave/for-next commit 26112f7f472

[David Sterba]

On Fri, Jul 08, 2016 at 07:55:47AM -0400, Jeff Mahoney wrote:
> >> The problem is that btrfs_calc_reclaim_metadata_size didn't used to be
> >> called from recovery, so it was safe to use fs_info->fs_root.  With
> >> commit 7c83c6a09 (Btrfs: don't bother kicking async if there's nothing
> >> to reclaim) we do call it from recovery context and fs_info->fs_root is
> >> NULL.
> >>
> >> The fix is to just not switch btrfs_calc_reclaim_metadata_size to take
> >> an fs_info.  All the other call sites were using fs_info->fs_root
> >> anyway, so it's not like we're pinning a root somewhere just for this call.
> > 
> > I've had this patch from last October in my 4.4.x tree forever:
> > http://www.spinics.net/lists/linux-btrfs/msg48457.html
> > 
> > Apparently it fell off the table. Shouldn't that fix it?
> 
> A different fix went into for-next.

Which JFI is https://patchwork.kernel.org/patch/8928981/ so the fix from
Tsutomu Itoh is not relevant anymore (but yeah it was lost in the
noise).