Re: [f2fs-dev] [PATCH] f2fs: get parent inode when recovering pino

[Chao Yu <yuchao0@huawei.com>]

On 2020/5/6 9:24, Eric Biggers wrote:
> On Wed, May 06, 2020 at 08:14:07AM +0800, Gao Xiang wrote:
>>>
>>> Actually, I think this is wrong because the fsync can be done via a file
>>> descriptor that was opened to a now-deleted link to the file.
>>
>> I'm still confused about this...
>>
>> I don't know what's wrong with this version from my limited knowledge?
>>  inode itself is locked when fsyncing, so
>>
>>    if the fsync inode->i_nlink == 1, this inode has only one hard link
>>    (not deleted yet) and should belong to a single directory; and
>>
>>    the only one parent directory would not go away (not deleted as well)
>>    since there are some dirents in it (not empty).
>>
>> Could kindly explain more so I would learn more about this scenario?
>> Thanks a lot!
> 
> i_nlink == 1 just means that there is one non-deleted link.  There can be links
> that have since been deleted, and file descriptors can still be open to them.
> 
>>
>>>
>>> We need to find the dentry whose parent directory is still exists, i.e. the
>>> parent directory that is counting towards 'inode->i_nlink == 1'.
>>
>> directory counting towards 'inode->i_nlink == 1', what's happening?
> 
> The non-deleted link is the one counted in i_nlink.
> 
>>
>>>
>>> I think d_find_alias() is what we're looking for.
>>
>> It may be simply dentry->d_parent (stable/positive as you said before, and it's
>> not empty). why need to d_find_alias()?
> 
> Because we need to get the dentry that hasn't been deleted yet, which isn't
> necessarily the one associated with the file descriptor being fsync()'ed.
> 
>> And what is the original problem? I could not get some clue from the original
>> patch description (I only saw some extra igrab/iput because of some unknown
>> reasons), it there some backtrace related to the problem?
> 
> The problem is that i_pino gets set incorrectly.  I just noticed this while
> reviewing the code.  It's not hard to reproduce, e.g.:
> 
> #include <unistd.h>
> #include <fcntl.h>
> #include <sys/stat.h>
> 
> int main()
> {
>         int fd;
> 
>         mkdir("dir1", 0700);
>         mkdir("dir2", 0700);
>         mknod("dir1/file", S_IFREG|0600, 0);
>         link("dir1/file", "dir2/file");
>         fd = open("dir2/file", O_WRONLY);
>         unlink("dir2/file");
>         write(fd, "X", 1);
>         fsync(fd);
> }
> 
> Then:
> 
> sync
> echo N | dump.f2fs -i $(stat -c %i dir1/file) /dev/vdb | grep 'i_pino'
> echo "dir1 (correct): $(stat -c %i dir1)"
> echo "dir2 (wrong): $(stat -c %i dir2)"
> 
> i_pino will point to dir2 rather than dir1 as expected.

Could you add above testcase into commit message of your patch? it will
be easier to understand the issue we solved with it.

In addition, how about adding this testcase in fstest as a generic one?

> 
> - Eric
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>