fs/fuse: set FATTR_FH flag on mtime file flush

Submitted by Pavel Butsykin on June 4, 2018, 1:57 p.m.

Details

Message ID 20180604135723.2957-1-pbutsykin@virtuozzo.com
State New
Series "fs/fuse: set FATTR_FH flag on mtime file flush"
Headers show

Commit Message

Pavel Butsykin June 4, 2018, 1:57 p.m.
If setattr request is for a file, FATTR_FH flag should be set.
In fuse_flush_mtime() that is clearly missed.

This fix is present in commit 1e18bda, but it wasn't backported because
the commit has a lot of unrelated changes. So this patch can be safely dropped
in case of moving to a newer kernel.

#VSTOR-10676

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
---
 fs/fuse/dir.c    | 6 +++++-
 fs/fuse/file.c   | 4 ++--
 fs/fuse/fuse_i.h | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 0ae0344be3c5..b04023bf230a 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1690,7 +1690,7 @@  static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
 /*
  * Flush inode->i_mtime to the server
  */
-int fuse_flush_mtime(struct file *file, bool nofail)
+int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail)
 {
 	struct inode *inode = file->f_mapping->host;
 	struct fuse_inode *fi = get_fuse_inode(inode);
@@ -1715,6 +1715,10 @@  int fuse_flush_mtime(struct file *file, bool nofail)
 	inarg.mtime = inode->i_mtime.tv_sec;
 	inarg.mtimensec = inode->i_mtime.tv_nsec;
 
+	if (ff) {
+		inarg.valid |= FATTR_FH;
+		inarg.fh = ff->fh;
+	}
 	fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
 	fuse_request_send(fc, req);
 	err = req->out.h.error;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 53e81bdca6ed..ddfb41af54ec 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -458,7 +458,7 @@  static int fuse_release(struct inode *inode, struct file *file)
 
 	if (test_bit(FUSE_I_MTIME_UPDATED,
 		     &get_fuse_inode(inode)->state))
-		fuse_flush_mtime(file, true);
+		fuse_flush_mtime(file, ff, true);
 
 	fuse_release_common(file, FUSE_RELEASE);
 
@@ -724,7 +724,7 @@  int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
 
 	if (!datasync && test_bit(FUSE_I_MTIME_UPDATED,
 				  &get_fuse_inode(inode)->state)) {
-		err = fuse_flush_mtime(file, false);
+		err = fuse_flush_mtime(file, isdir ? NULL : ff, false);
 		if (err)
 			goto out;
 	}
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f704fd17905b..939835f585b1 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1074,7 +1074,7 @@  int fuse_dev_release(struct inode *inode, struct file *file);
 
 bool fuse_write_update_size(struct inode *inode, loff_t pos);
 
-int fuse_flush_mtime(struct file *file, bool nofail);
+int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail);
 
 int fuse_do_setattr(struct inode *inode, struct iattr *attr,
 		    struct file *file);

Comments

Kirill Tkhai June 5, 2018, 9:53 a.m.
On 04.06.2018 16:57, Pavel Butsykin wrote:
> If setattr request is for a file, FATTR_FH flag should be set.
> In fuse_flush_mtime() that is clearly missed.
> 
> This fix is present in commit 1e18bda, but it wasn't backported because
> the commit has a lot of unrelated changes. So this patch can be safely dropped
> in case of moving to a newer kernel.
> 
> #VSTOR-10676
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  fs/fuse/dir.c    | 6 +++++-
>  fs/fuse/file.c   | 4 ++--
>  fs/fuse/fuse_i.h | 2 +-
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 0ae0344be3c5..b04023bf230a 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1690,7 +1690,7 @@ static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
>  /*
>   * Flush inode->i_mtime to the server
>   */
> -int fuse_flush_mtime(struct file *file, bool nofail)
> +int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail)
>  {
>  	struct inode *inode = file->f_mapping->host;
>  	struct fuse_inode *fi = get_fuse_inode(inode);
> @@ -1715,6 +1715,10 @@ int fuse_flush_mtime(struct file *file, bool nofail)
>  	inarg.mtime = inode->i_mtime.tv_sec;
>  	inarg.mtimensec = inode->i_mtime.tv_nsec;
>  
> +	if (ff) {
> +		inarg.valid |= FATTR_FH;
> +		inarg.fh = ff->fh;
> +	}
>  	fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
>  	fuse_request_send(fc, req);
>  	err = req->out.h.error;
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 53e81bdca6ed..ddfb41af54ec 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -458,7 +458,7 @@ static int fuse_release(struct inode *inode, struct file *file)
>  
>  	if (test_bit(FUSE_I_MTIME_UPDATED,
>  		     &get_fuse_inode(inode)->state))
> -		fuse_flush_mtime(file, true);
> +		fuse_flush_mtime(file, ff, true);
>  
>  	fuse_release_common(file, FUSE_RELEASE);
>  
> @@ -724,7 +724,7 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>  
>  	if (!datasync && test_bit(FUSE_I_MTIME_UPDATED,
>  				  &get_fuse_inode(inode)->state)) {
> -		err = fuse_flush_mtime(file, false);
> +		err = fuse_flush_mtime(file, isdir ? NULL : ff, false);

Everything looks OK for me, and the only thing I want to ask you is the reason
we ignore ff in case of directory. Why we should do this?

>  		if (err)
>  			goto out;
>  	}
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index f704fd17905b..939835f585b1 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1074,7 +1074,7 @@ int fuse_dev_release(struct inode *inode, struct file *file);
>  
>  bool fuse_write_update_size(struct inode *inode, loff_t pos);
>  
> -int fuse_flush_mtime(struct file *file, bool nofail);
> +int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail);
>  
>  int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>  		    struct file *file);
>
Pavel Butsykin June 5, 2018, 10:05 a.m.
On 05.06.2018 12:53, Kirill Tkhai wrote:
> On 04.06.2018 16:57, Pavel Butsykin wrote:
>> If setattr request is for a file, FATTR_FH flag should be set.
>> In fuse_flush_mtime() that is clearly missed.
>>
>> This fix is present in commit 1e18bda, but it wasn't backported because
>> the commit has a lot of unrelated changes. So this patch can be safely dropped
>> in case of moving to a newer kernel.
>>
>> #VSTOR-10676
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> ---
>>   fs/fuse/dir.c    | 6 +++++-
>>   fs/fuse/file.c   | 4 ++--
>>   fs/fuse/fuse_i.h | 2 +-
>>   3 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>> index 0ae0344be3c5..b04023bf230a 100644
>> --- a/fs/fuse/dir.c
>> +++ b/fs/fuse/dir.c
>> @@ -1690,7 +1690,7 @@ static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
>>   /*
>>    * Flush inode->i_mtime to the server
>>    */
>> -int fuse_flush_mtime(struct file *file, bool nofail)
>> +int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail)
>>   {
>>   	struct inode *inode = file->f_mapping->host;
>>   	struct fuse_inode *fi = get_fuse_inode(inode);
>> @@ -1715,6 +1715,10 @@ int fuse_flush_mtime(struct file *file, bool nofail)
>>   	inarg.mtime = inode->i_mtime.tv_sec;
>>   	inarg.mtimensec = inode->i_mtime.tv_nsec;
>>   
>> +	if (ff) {
>> +		inarg.valid |= FATTR_FH;
>> +		inarg.fh = ff->fh;
>> +	}
>>   	fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
>>   	fuse_request_send(fc, req);
>>   	err = req->out.h.error;
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index 53e81bdca6ed..ddfb41af54ec 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -458,7 +458,7 @@ static int fuse_release(struct inode *inode, struct file *file)
>>   
>>   	if (test_bit(FUSE_I_MTIME_UPDATED,
>>   		     &get_fuse_inode(inode)->state))
>> -		fuse_flush_mtime(file, true);
>> +		fuse_flush_mtime(file, ff, true);
>>   
>>   	fuse_release_common(file, FUSE_RELEASE);
>>   
>> @@ -724,7 +724,7 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>>   
>>   	if (!datasync && test_bit(FUSE_I_MTIME_UPDATED,
>>   				  &get_fuse_inode(inode)->state)) {
>> -		err = fuse_flush_mtime(file, false);
>> +		err = fuse_flush_mtime(file, isdir ? NULL : ff, false);
> 
> Everything looks OK for me, and the only thing I want to ask you is the reason
> we ignore ff in case of directory. Why we should do this?

Because FATTR_FH flag indicates that request belongs to a file.
Correspondingly, this flag should be set for a directory.

> 
>>   		if (err)
>>   			goto out;
>>   	}
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index f704fd17905b..939835f585b1 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -1074,7 +1074,7 @@ int fuse_dev_release(struct inode *inode, struct file *file);
>>   
>>   bool fuse_write_update_size(struct inode *inode, loff_t pos);
>>   
>> -int fuse_flush_mtime(struct file *file, bool nofail);
>> +int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail);
>>   
>>   int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>>   		    struct file *file);
>>
Kirill Tkhai June 5, 2018, 10:17 a.m.
On 04.06.2018 16:57, Pavel Butsykin wrote:
> If setattr request is for a file, FATTR_FH flag should be set.
> In fuse_flush_mtime() that is clearly missed.
> 
> This fix is present in commit 1e18bda, but it wasn't backported because
> the commit has a lot of unrelated changes. So this patch can be safely dropped
> in case of moving to a newer kernel.
> 
> #VSTOR-10676
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  fs/fuse/dir.c    | 6 +++++-
>  fs/fuse/file.c   | 4 ++--
>  fs/fuse/fuse_i.h | 2 +-
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 0ae0344be3c5..b04023bf230a 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1690,7 +1690,7 @@ static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
>  /*
>   * Flush inode->i_mtime to the server
>   */
> -int fuse_flush_mtime(struct file *file, bool nofail)
> +int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail)
>  {
>  	struct inode *inode = file->f_mapping->host;
>  	struct fuse_inode *fi = get_fuse_inode(inode);
> @@ -1715,6 +1715,10 @@ int fuse_flush_mtime(struct file *file, bool nofail)
>  	inarg.mtime = inode->i_mtime.tv_sec;
>  	inarg.mtimensec = inode->i_mtime.tv_nsec;
>  
> +	if (ff) {
> +		inarg.valid |= FATTR_FH;
> +		inarg.fh = ff->fh;
> +	}
>  	fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
>  	fuse_request_send(fc, req);
>  	err = req->out.h.error;
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 53e81bdca6ed..ddfb41af54ec 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -458,7 +458,7 @@ static int fuse_release(struct inode *inode, struct file *file)
>  
>  	if (test_bit(FUSE_I_MTIME_UPDATED,
>  		     &get_fuse_inode(inode)->state))
> -		fuse_flush_mtime(file, true);
> +		fuse_flush_mtime(file, ff, true);
>  
>  	fuse_release_common(file, FUSE_RELEASE);
>  
> @@ -724,7 +724,7 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>  
>  	if (!datasync && test_bit(FUSE_I_MTIME_UPDATED,
>  				  &get_fuse_inode(inode)->state)) {
> -		err = fuse_flush_mtime(file, false);
> +		err = fuse_flush_mtime(file, isdir ? NULL : ff, false);
>  		if (err)
>  			goto out;
>  	}
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index f704fd17905b..939835f585b1 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1074,7 +1074,7 @@ int fuse_dev_release(struct inode *inode, struct file *file);
>  
>  bool fuse_write_update_size(struct inode *inode, loff_t pos);
>  
> -int fuse_flush_mtime(struct file *file, bool nofail);
> +int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail);
>  
>  int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>  		    struct file *file);
>
Kirill Tkhai June 5, 2018, 10:28 a.m.
On 05.06.2018 13:05, Pavel Butsykin wrote:
> On 05.06.2018 12:53, Kirill Tkhai wrote:
>> On 04.06.2018 16:57, Pavel Butsykin wrote:
>>> If setattr request is for a file, FATTR_FH flag should be set.
>>> In fuse_flush_mtime() that is clearly missed.
>>>
>>> This fix is present in commit 1e18bda, but it wasn't backported because
>>> the commit has a lot of unrelated changes. So this patch can be safely dropped
>>> in case of moving to a newer kernel.
>>>
>>> #VSTOR-10676
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> ---
>>>   fs/fuse/dir.c    | 6 +++++-
>>>   fs/fuse/file.c   | 4 ++--
>>>   fs/fuse/fuse_i.h | 2 +-
>>>   3 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>>> index 0ae0344be3c5..b04023bf230a 100644
>>> --- a/fs/fuse/dir.c
>>> +++ b/fs/fuse/dir.c
>>> @@ -1690,7 +1690,7 @@ static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
>>>   /*
>>>    * Flush inode->i_mtime to the server
>>>    */
>>> -int fuse_flush_mtime(struct file *file, bool nofail)
>>> +int fuse_flush_mtime(struct file *file, struct fuse_file *ff, bool nofail)
>>>   {
>>>       struct inode *inode = file->f_mapping->host;
>>>       struct fuse_inode *fi = get_fuse_inode(inode);
>>> @@ -1715,6 +1715,10 @@ int fuse_flush_mtime(struct file *file, bool nofail)
>>>       inarg.mtime = inode->i_mtime.tv_sec;
>>>       inarg.mtimensec = inode->i_mtime.tv_nsec;
>>>   +    if (ff) {
>>> +        inarg.valid |= FATTR_FH;
>>> +        inarg.fh = ff->fh;
>>> +    }
>>>       fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
>>>       fuse_request_send(fc, req);
>>>       err = req->out.h.error;
>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>> index 53e81bdca6ed..ddfb41af54ec 100644
>>> --- a/fs/fuse/file.c
>>> +++ b/fs/fuse/file.c
>>> @@ -458,7 +458,7 @@ static int fuse_release(struct inode *inode, struct file *file)
>>>         if (test_bit(FUSE_I_MTIME_UPDATED,
>>>                &get_fuse_inode(inode)->state))
>>> -        fuse_flush_mtime(file, true);
>>> +        fuse_flush_mtime(file, ff, true);
>>>         fuse_release_common(file, FUSE_RELEASE);
>>>   @@ -724,7 +724,7 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>>>         if (!datasync && test_bit(FUSE_I_MTIME_UPDATED,
>>>                     &get_fuse_inode(inode)->state)) {
>>> -        err = fuse_flush_mtime(file, false);
>>> +        err = fuse_flush_mtime(file, isdir ? NULL : ff, false);
>>
>> Everything looks OK for me, and the only thing I want to ask you is the reason
>> we ignore ff in case of directory. Why we should do this?
> 
> Because FATTR_FH flag indicates that request belongs to a file.
> Correspondingly, this flag should be set for a directory.

It looks like MS may set this flag for both cases. Let's keep it for file only now,
since we have no a real workload to test this (and we don't want to break anything).
If someone needs this, we add it for directory too.