Re: [PATCH 5/5] f2fs: Fix for parent inode information during server cache eviction

[Jaegeuk Kim <jaegeuk.kim@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4278 bytes --]

2012-12-10 (월), 16:11 +0900, Namjae Jeon:
> 2012/12/10, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> > 2012-12-10 (월), 14:25 +0900, Namjae Jeon:
> >> 2012/12/10, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> >> > 2012-12-10 (월), 12:40 +0900, Namjae Jeon:
> >> >> 2012/12/10, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> >> >> > 2012-12-08 (토), 14:55 +0900, Namjae Jeon:
> >> >> >> From: Namjae Jeon <namjae.jeon@samsung.com>
> >> >> >>
> >> >> >> Test Case:
> >> >> >> [NFS Client]
> >> >> >> ls -lR .
> >> >> >>
> >> >> >> [NFS Server]
> >> >> >> while [ 1 ]
> >> >> >> do
> >> >> >> echo 3 > /proc/sys/vm/drop_caches
> >> >> >> done
> >> >> >>
> >> >> >> Error: "No such file or directory"
> >> >> >>
> >> >> >> When cache is dropped at the server, it results in lookup failure
> >> >> >> at
> >> >> >> the
> >> >> >> NFS client. Even though the file exists. Looking at the code to
> >> >> >> rebuild
> >> >> >> the inode in case of cache eviction. It tries to initiate a lookup
> >> >> >> operation
> >> >> >> for ".." to get the parent information using the on-disk inode
> >> >> >> number.
> >> >> >>
> >> >> >
> >> >> > Could you describe why this patch resolves that bug?
> >> >> > Before applying this, we need to figure out why that bug is
> >> >> > occurred.
> >> >> > IMO, from the viewpoint of functionality, ".." resolution should
> >> >> > work
> >> >> > too.
> >> >> dotdot entry of f2fs is stored when creating only directory not
> >> >> regular file. Am I correct ?
> >> >
> >> > Yep.
> >> >
> >> >> So when the parent of file was evicted, I thought we could not get
> >> >> parent inode number of file thoughout dotdot entry.
> >> >
> >> > What do you mean the parent of file? Isn't it a directory?
> >> >
> >> >> And f2fs inode is having parent inode number unlike other fs. so I
> >> >> think we can use this special thing by storing f2fs_inode_info.
> >> >
> >> > f2fs stores a dotdot dentry *likewise* other fs.
> >> > The pino in f2fs_inode is specially added for POR intentionally.
> >> >
> >> > Still I cannot imagine the bug scenario.
> >> When we observed the issue in NFS operations. We found that the issues
> >> occurs while trying to “reconnect” with parent ? and shows “No such
> >> file or directory” for directory paths.
> >
> > Again, you didn't describe why this bug is occurred.
> > Please, analyze the bug scenario first.
> Okay, I will look into more.
> >
> >>
> >> As a matter of fact - we were also checking the on-disk layout for the
> >> f2fs_inode. We found that since, F2FS inode is keeping a reference to
> >> the parent-inode within its on-disk information. So, we should use the
> >> same information to reconstruct the parent in case of eviction.
> >>
> >> The benefit is:
> >> It also saves the lookup() to be performed for “..” in the directory
> >> entry block while trying to reconnect with the parent. Instead we can
> >> directly use the parent inode number in the inode and generate from
> >> that point itself.  Even though the get_parent() code was made similar
> >> to other filesystem which generally do not have reference to the
> >> parent inode -  so they need to perform a lookup for “..”, but in this
> >> case we sincerely thought we can get rid of that method.
> >> So, we did not tried to figure out what could be possible solution in
> >> the current scenario.
> >> As per your reference, parent inode number for on-disk inode is
> >> introduced for POR intentionally. So, we should not use information
> >> for any reconstruction?
> >
> > Of course, we can enhance the directory operations by using the "pino".
> > But what I concern is how to resolve the bug by this enhancement patch.
> > This is not a bug fix patch. Isn't it?
> Yes, It is not bug fix. this is new method using f2fs's special thing.
> Agreed, I will analyse why this bug occur first. let's discuss again
> whether we fix bug or use new method.
> 
> And please check the rest of patches except this patch.

Other patches look good to me.
And I'm testing and seeing the codes one more time.
Thank you for contribution. :)

> 
> Thanks.
> >
> > Thanks,
> >
> > --
> > Jaegeuk Kim
> > Samsung
> >

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]