[Cluster-devel] [PATCH 4/4] GFS2: Make deletes use regular inode_lookup with special code path

[Steven Whitehouse <swhiteho@redhat.com>]

On 24/02/16 20:42, Bob Peterson wrote:
> ----- Original Message -----
>> Hi,
>>
>> On 18/12/15 19:02, Bob Peterson wrote:
>>> This patch changes the inode reclaiming code, delete_work_func, use
>>> the normal gfs2_inode_lookup, but with a special parameter, for_del
>>> which causes additional checks to be made.
>> I assume that this was just intended to be a clean up rather than
>> something that fixes a bug? At least I can't see any obvious changes to
>> the operations, just the way in which they are carried out. I think it
>> would be neater to turn the non_block parameter of gfs2_inode_lookup()
>> into a flags argument, rather than adding an additional argument which
>> is only a bool.
>>
>> I'm not sure that factoring out some of the checks from
>> gfs2_lookup_by_inum() buys us that much though, since that function
>> still has to remain in its current form for NFS anyway,
>>
>> Steve.
> Hi Steve,
>
> Actually, no, it's not a cleanup at all. Without this patch, I can
> recreate one of the problems I'm trying to solve here, which is:
>
> [nate_bob1] [fsck] Unlinked inode found at block 483992 (0x76298).
> [nate_bob1] [fsck] Block 483993 (0x76299) bitmap says 1 (Data) but FSCK saw 0 (Free)
> [nate_bob1] [fsck] Bitmap at block 483993 (0x76299) left inconsistent
> [nate_bob1] [fsck] Block 483994 (0x7629a) bitmap says 1 (Data) but FSCK saw 0 (Free)
> [nate_bob1] [fsck] Bitmap at block 483994 (0x7629a) left inconsistent
> [nate_bob1] [fsck] Block 483995 (0x7629b) bitmap says 1 (Data) but FSCK saw 0 (Free)
> [nate_bob1] [fsck] Bitmap at block 483995 (0x7629b) left inconsistent
> [nate_bob1] [fsck] Block 483996 (0x7629c) bitmap says 1 (Data) but FSCK saw 0 (Free)
> [nate_bob1] [fsck] Bitmap at block 483996 (0x7629c) left inconsistent
> [nate_bob1] [fsck] Block 483997 (0x7629d) bitmap says 1 (Data) but FSCK saw 0 (Free)
> [nate_bob1] [fsck] Bitmap at block 483997 (0x7629d) left inconsistent
> etc.
>
> Turns out if you use the regular inode lookup path, the problem goes away.
>
> Initially (several months ago) I had written a new inode lookup function
> just for the transition from unlinked to free. I debugged it and got it
> working fairly well, only to discover that my code did the exact same things
> in the exact same order as the original lookup. So I deleted my new function
> in favor of using the original, hence, this patch.
>
> My concern, of course, is that NFS may also be vulnerable to this problem,
> but I didn't want to mess with the NFS path any more than I had to, so I
> left the original inum lookup there for it to use. At some point we should
> try to determine if NFS has an issue related to this. I suspect it does.
>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
I think we need to know the answer to that before we make this change. 
Also, what is the problem? I'm still not sure that I understand the 
actual issue here. It is down to the ordering of operations, or 
different locking, or what exactly?

The fsck output seems to indicate that there is an unlinked inode in 
which some of the blocks are marked as free. If that occurs without 
there being an un-recovered journal then that is certainly a problem, 
but it would be with the transaction code, so I'm still not quite sure 
how this patch relates to it. It looks like it needs more investigation,

Steve.