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

[Amir Goldstein]

On Fri, Sep 16, 2016 at 7:38 PM, Jan Kara  wrote:
> On Fri 16-09-16 14:19:23, Miklos Szeredi wrote:
>> From: Aihua Zhang 
>>
>> 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 
>> Signed-off-by: Miklos Szeredi 
>> Cc: Jan Kara 
>> Cc: Eric Paris 
>> ---
>>  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 = >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.

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

[Amir Goldstein]

On Fri, Sep 16, 2016 at 7:38 PM, Jan Kara  wrote:
> On Fri 16-09-16 14:19:23, Miklos Szeredi wrote:
>> From: Aihua Zhang 
>>
>> 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 
>> Signed-off-by: Miklos Szeredi 
>> Cc: Jan Kara 
>> Cc: Eric Paris 
>> ---
>>  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 = >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.

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

[Jan Kara]

On Fri 16-09-16 14:19:23, Miklos Szeredi wrote:
> From: Aihua Zhang 
> 
> 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 
> Signed-off-by: Miklos Szeredi 
> Cc: Jan Kara 
> Cc: Eric Paris 
> ---
>  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 = >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?

Honza
-- 
Jan Kara 
SUSE Labs, CR

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

[Jan Kara]

On Fri 16-09-16 14:19:23, Miklos Szeredi wrote:
> From: Aihua Zhang 
> 
> 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 
> Signed-off-by: Miklos Szeredi 
> Cc: Jan Kara 
> Cc: Eric Paris 
> ---
>  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 = >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?

Honza
-- 
Jan Kara 
SUSE Labs, CR

[PATCH 04/12] fsnotify: support overlayfs

[Miklos Szeredi]

From: Aihua Zhang 

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 
Signed-off-by: Miklos Szeredi 
Cc: Jan Kara 
Cc: Eric Paris 
---
 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 = >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;
__u32 fsnotify_mask = 0;
int ret;
 
@@ -173,7 +177,7 @@ static inline void fsnotify_mkdir(struct inode *inode, 
struct dentry *dentry)
 static inline void fsnotify_access(struct file *file)
 {
struct path *path = >f_path;
-   struct inode *inode = file_inode(file);
+   struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_ACCESS;
 
if (S_ISDIR(inode->i_mode))
@@ -191,7 +195,7 @@ static inline void fsnotify_access(struct file *file)
 static inline void fsnotify_modify(struct file *file)
 {
struct path *path = >f_path;
-   struct inode *inode = file_inode(file);
+   struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_MODIFY;
 
if (S_ISDIR(inode->i_mode))
@@ -209,7 +213,7 @@ static inline void fsnotify_modify(struct file *file)
 static inline void fsnotify_open(struct file *file)
 {
struct path *path = >f_path;
-   struct inode *inode = file_inode(file);
+   struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_OPEN;
 
if (S_ISDIR(inode->i_mode))
@@ -225,7 +229,7 @@ static inline void fsnotify_open(struct file *file)
 static inline void fsnotify_close(struct file *file)
 {
struct path *path = >f_path;
-   struct inode *inode = file_inode(file);
+   struct inode *inode = path->dentry->d_inode;
fmode_t mode = file->f_mode;
__u32 mask = (mode & FMODE_WRITE) ? FS_CLOSE_WRITE : FS_CLOSE_NOWRITE;
 
-- 
2.5.5

[PATCH 04/12] fsnotify: support overlayfs

[Miklos Szeredi]

From: Aihua Zhang 

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 
Signed-off-by: Miklos Szeredi 
Cc: Jan Kara 
Cc: Eric Paris 
---
 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 = >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;
__u32 fsnotify_mask = 0;
int ret;
 
@@ -173,7 +177,7 @@ static inline void fsnotify_mkdir(struct inode *inode, 
struct dentry *dentry)
 static inline void fsnotify_access(struct file *file)
 {
struct path *path = >f_path;
-   struct inode *inode = file_inode(file);
+   struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_ACCESS;
 
if (S_ISDIR(inode->i_mode))
@@ -191,7 +195,7 @@ static inline void fsnotify_access(struct file *file)
 static inline void fsnotify_modify(struct file *file)
 {
struct path *path = >f_path;
-   struct inode *inode = file_inode(file);
+   struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_MODIFY;
 
if (S_ISDIR(inode->i_mode))
@@ -209,7 +213,7 @@ static inline void fsnotify_modify(struct file *file)
 static inline void fsnotify_open(struct file *file)
 {
struct path *path = >f_path;
-   struct inode *inode = file_inode(file);
+   struct inode *inode = path->dentry->d_inode;
__u32 mask = FS_OPEN;
 
if (S_ISDIR(inode->i_mode))
@@ -225,7 +229,7 @@ static inline void fsnotify_open(struct file *file)
 static inline void fsnotify_close(struct file *file)
 {
struct path *path = >f_path;
-   struct inode *inode = file_inode(file);
+   struct inode *inode = path->dentry->d_inode;
fmode_t mode = file->f_mode;
__u32 mask = (mode & FMODE_WRITE) ? FS_CLOSE_WRITE : FS_CLOSE_NOWRITE;
 
-- 
2.5.5