Re: [PATCH] Btrfs: optimize btrfs_lookup_extent_info() - extent refcount mismatches?

Re: [PATCH] Btrfs: optimize btrfs_lookup_extent_info() - extent refcount mismatches?

[David Sterba]

Hi,

On Fri, Jul 05, 2013 at 09:32:05PM +0100, Filipe David Borba Manana wrote:
> If we're looking for a metadata item in the tree and the
> search fails with return value of 1, and the slot doesn't
> point to the first item in the leaf, check if the previous
> item in the leaf corresponds to an extent item for the same
> object id - if it does, then don't do another tree search
> to get it.

I'm suspecting this patch to cause some trouble, see
https://bugzilla.kernel.org/show_bug.cgi?id=64961

but there were more reports similar to this one:
http://www.spinics.net/lists/linux-btrfs/msg29243.html
http://www.spinics.net/lists/linux-btrfs/msg29478.html

I did a quick analysis in comment 3, the warning pops when there are 0
references, there's no other evidence that the extent should have none.
This happens during balance, so far the working hypothesis is that the
refcount changes during relocation (eg. subvolume deletion) but is not
fixed up in the late reloc phase.

fsck finds lots of wrong refcounts, off by one. The kernel warning fires
only if the refcount is 0 though there are > 0 expected. So this could
mean that the refcount is silently wrong all the time.


david

Re: [PATCH] Btrfs: optimize btrfs_lookup_extent_info() - extent refcount mismatches?

[Filipe David Manana]

On Fri, Nov 29, 2013 at 3:45 PM, David Sterba <dsterba@suse.cz> wrote:
> Hi,
>
> On Fri, Jul 05, 2013 at 09:32:05PM +0100, Filipe David Borba Manana wrote:
>> If we're looking for a metadata item in the tree and the
>> search fails with return value of 1, and the slot doesn't
>> point to the first item in the leaf, check if the previous
>> item in the leaf corresponds to an extent item for the same
>> object id - if it does, then don't do another tree search
>> to get it.
>
> I'm suspecting this patch to cause some trouble, see
> https://bugzilla.kernel.org/show_bug.cgi?id=64961

Hi David,

What makes you believe the problem is exactly in this function?

I read it again, and I can't see how it can miss an extent item that
it couldn't before, specially without skinny metadata enabled.  Did
the fs had skinny metadata enabled?

thanks


>
> but there were more reports similar to this one:
> http://www.spinics.net/lists/linux-btrfs/msg29243.html
> http://www.spinics.net/lists/linux-btrfs/msg29478.html
>
> I did a quick analysis in comment 3, the warning pops when there are 0
> references, there's no other evidence that the extent should have none.
> This happens during balance, so far the working hypothesis is that the
> refcount changes during relocation (eg. subvolume deletion) but is not
> fixed up in the late reloc phase.
>
> fsck finds lots of wrong refcounts, off by one. The kernel warning fires
> only if the refcount is 0 though there are > 0 expected. So this could
> mean that the refcount is silently wrong all the time.
>
>
> david



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] Btrfs: optimize btrfs_lookup_extent_info() - extent refcount mismatches?

[David Sterba]

On Fri, Nov 29, 2013 at 07:55:37PM +0000, Filipe David Manana wrote:
> On Fri, Nov 29, 2013 at 3:45 PM, David Sterba <dsterba@suse.cz> wrote:
> > Hi,
> >
> > On Fri, Jul 05, 2013 at 09:32:05PM +0100, Filipe David Borba Manana wrote:
> >> If we're looking for a metadata item in the tree and the
> >> search fails with return value of 1, and the slot doesn't
> >> point to the first item in the leaf, check if the previous
> >> item in the leaf corresponds to an extent item for the same
> >> object id - if it does, then don't do another tree search
> >> to get it.
> >
> > I'm suspecting this patch to cause some trouble, see
> > https://bugzilla.kernel.org/show_bug.cgi?id=64961
> 
> What makes you believe the problem is exactly in this function?

It was a first guess based on the stacktraces and last patch that
touched the code. I've replied to mailinlist because it has higher
visibility, number of bugreports in balance/relocation has been
increasing lately (not blaming you).

> I read it again, and I can't see how it can miss an extent item that
> it couldn't before, specially without skinny metadata enabled.  Did
> the fs had skinny metadata enabled?

According to the updated bugzilla yes, and that's another possible
cause although we haven't seen bugs with skinny metadata yet. There's
another bugreport that has them enabled,
https://bugzilla.kernel.org/show_bug.cgi?id=66411


david

Re: [PATCH] Btrfs: optimize btrfs_lookup_extent_info() - extent refcount mismatches?

[Josef Bacik]

On Tue, Dec 3, 2013 at 8:43 AM, David Sterba <dsterba@suse.cz> wrote:
> On Fri, Nov 29, 2013 at 07:55:37PM +0000, Filipe David Manana wrote:
>> On Fri, Nov 29, 2013 at 3:45 PM, David Sterba <dsterba@suse.cz> wrote:
>> > Hi,
>> >
>> > On Fri, Jul 05, 2013 at 09:32:05PM +0100, Filipe David Borba Manana wrote:
>> >> If we're looking for a metadata item in the tree and the
>> >> search fails with return value of 1, and the slot doesn't
>> >> point to the first item in the leaf, check if the previous
>> >> item in the leaf corresponds to an extent item for the same
>> >> object id - if it does, then don't do another tree search
>> >> to get it.
>> >
>> > I'm suspecting this patch to cause some trouble, see
>> > https://bugzilla.kernel.org/show_bug.cgi?id=64961
>>
>> What makes you believe the problem is exactly in this function?
>
> It was a first guess based on the stacktraces and last patch that
> touched the code. I've replied to mailinlist because it has higher
> visibility, number of bugreports in balance/relocation has been
> increasing lately (not blaming you).
>
>> I read it again, and I can't see how it can miss an extent item that
>> it couldn't before, specially without skinny metadata enabled.  Did
>> the fs had skinny metadata enabled?
>
> According to the updated bugzilla yes, and that's another possible
> cause although we haven't seen bugs with skinny metadata yet. There's
> another bugreport that has them enabled,
> https://bugzilla.kernel.org/show_bug.cgi?id=66411
>

I've been seeing similar issues while testing my qgroups patches that
I don't think is related to my qgroup work.  I'm going to try and
track it down this week but I may not get to it until next week.
Thanks,

Josef

Re: [PATCH] Btrfs: optimize btrfs_lookup_extent_info() - extent refcount mismatches?

[Filipe David Manana]

On Tue, Dec 3, 2013 at 6:28 PM, Josef Bacik <josef@toxicpanda.com> wrote:
> On Tue, Dec 3, 2013 at 8:43 AM, David Sterba <dsterba@suse.cz> wrote:
>> On Fri, Nov 29, 2013 at 07:55:37PM +0000, Filipe David Manana wrote:
>>> On Fri, Nov 29, 2013 at 3:45 PM, David Sterba <dsterba@suse.cz> wrote:
>>> > Hi,
>>> >
>>> > On Fri, Jul 05, 2013 at 09:32:05PM +0100, Filipe David Borba Manana wrote:
>>> >> If we're looking for a metadata item in the tree and the
>>> >> search fails with return value of 1, and the slot doesn't
>>> >> point to the first item in the leaf, check if the previous
>>> >> item in the leaf corresponds to an extent item for the same
>>> >> object id - if it does, then don't do another tree search
>>> >> to get it.
>>> >
>>> > I'm suspecting this patch to cause some trouble, see
>>> > https://bugzilla.kernel.org/show_bug.cgi?id=64961
>>>
>>> What makes you believe the problem is exactly in this function?
>>
>> It was a first guess based on the stacktraces and last patch that
>> touched the code. I've replied to mailinlist because it has higher
>> visibility, number of bugreports in balance/relocation has been
>> increasing lately (not blaming you).
>>
>>> I read it again, and I can't see how it can miss an extent item that
>>> it couldn't before, specially without skinny metadata enabled.  Did
>>> the fs had skinny metadata enabled?
>>
>> According to the updated bugzilla yes, and that's another possible
>> cause although we haven't seen bugs with skinny metadata yet. There's
>> another bugreport that has them enabled,
>> https://bugzilla.kernel.org/show_bug.cgi?id=66411

I found an issue in btrfs_lookup_extent_info() when skinny metadata is
enabled. It can actually miss existing skinny extent items.
The following patch fixes it and explains in detail the issue:

https://patchwork.kernel.org/patch/3305521/

thanks

>>
>
> I've been seeing similar issues while testing my qgroups patches that
> I don't think is related to my qgroup work.  I'm going to try and
> track it down this week but I may not get to it until next week.
> Thanks,
>
> Josef



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."