Re: [PATCH 04/12] fsnotify: support overlayfs

[Amir Goldstein]

On Fri, Sep 16, 2016 at 7:38 PM, Jan Kara <jack@xxxxxxx> wrote:
> On Fri 16-09-16 14:19:23, Miklos Szeredi wrote:
>> From: Aihua Zhang <zhangaihua1@xxxxxxxxxx>
>>
>> When an event occurs direct it to the overlay inode instead of the real
>> underlying inode.
>>
>> This will work even if the file was first on the lower layer and then
>> copied up, while the watch is there.  This is because the watch is on the
>> overlay inode, which stays the same through the copy-up.
>>
>> For filesystems other than overlayfs this is a no-op, except for the
>> performance impact of an extra pointer dereferece.
>>
>> Verified to work correctly with the inotify/fanotify tests in LTP.
>>
>> Signed-off-by: Aihua Zhang <zhangaihua1@xxxxxxxxxx>
>> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
>> Cc: Jan Kara <jack@xxxxxxx>
>> Cc: Eric Paris <eparis@xxxxxxxxxx>
>> ---
>>  include/linux/fsnotify.h | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
>> index eed9e853a06f..b8bcc058e031 100644
>> --- a/include/linux/fsnotify.h
>> +++ b/include/linux/fsnotify.h
>> @@ -29,7 +29,11 @@ static inline int fsnotify_parent(struct path *path, struct dentry *dentry, __u3
>>  static inline int fsnotify_perm(struct file *file, int mask)
>>  {
>>       struct path *path = &file->f_path;
>> -     struct inode *inode = file_inode(file);
>> +     /*
>> +      * Do not use file_inode() here or anywhere in this file to get the
>> +      * inode.  That would break *notity on overlayfs.
>> +      */
>> +     struct inode *inode = path->dentry->d_inode;
>
> So shouldn't we rather have d_backing_inode(path->dentry) here and
> everywhere else?

d_backing_inode()'s intention is quite the opposite
it's documented as "Get upper or lower inode we should be using"

So there is no helper to return this "unreal" inode.
In fact, it seems that was the intention of d_inode() helper,
but it won't work for overlayfs.

Miklos just submitted a new helper locks_inode() for 4.9, which does
exactly that.
I suppose it's either another similar helper fsnotify_inode() or find
a better and generic
name for this creature.

IMO, the helper file_dentry(), that Miklos introduced in commit d101a1259
should be renamed to file_backing_dentry() and a new helper file_dentry()
should be used here. i.e. inode = d_inode(file_dentry(file));

Also in this case, I think that an explicit name for the helper to get
the "unreal" dentry
would serve better then a generic name, but I can't think of a better name...

Cheers,
Amir.