diff mbox series

[v7,10/15] fs: Add async write file modification handling.

Message ID 20220601210141.3773402-11-shr@fb.com (mailing list archive)
State New, archived
Headers show
Series io-uring/xfs: support async buffered writes | expand

Commit Message

Stefan Roesch June 1, 2022, 9:01 p.m. UTC 
This adds a file_modified_async() function to return -EAGAIN if the
request either requires to remove privileges or needs to update the file
modification time. This is required for async buffered writes, so the
request gets handled in the io worker of io-uring.

Signed-off-by: Stefan Roesch <shr@fb.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/inode.c         | 43 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/fs.h |  1 +
 2 files changed, 42 insertions(+), 2 deletions(-)

Comments

Jan Kara June 2, 2022, 8:44 a.m. UTC | #1 
On Wed 01-06-22 14:01:36, Stefan Roesch wrote:
> This adds a file_modified_async() function to return -EAGAIN if the
> request either requires to remove privileges or needs to update the file
> modification time. This is required for async buffered writes, so the
> request gets handled in the io worker of io-uring.
> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/inode.c         | 43 +++++++++++++++++++++++++++++++++++++++++--
>  include/linux/fs.h |  1 +
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index c44573a32c6a..4503bed063e7 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2116,17 +2116,21 @@ int file_update_time(struct file *file)
>  EXPORT_SYMBOL(file_update_time);
>  
>  /**
> - * file_modified - handle mandated vfs changes when modifying a file
> + * file_modified_flags - handle mandated vfs changes when modifying a file
>   * @file: file that was modified
> + * @flags: kiocb flags
>   *
>   * When file has been modified ensure that special
>   * file privileges are removed and time settings are updated.
>   *
> + * If IOCB_NOWAIT is set, special file privileges will not be removed and
> + * time settings will not be updated. It will return -EAGAIN.
> + *
>   * Context: Caller must hold the file's inode lock.
>   *
>   * Return: 0 on success, negative errno on failure.
>   */
> -int file_modified(struct file *file)
> +static int file_modified_flags(struct file *file, int flags)
>  {
>  	int ret;
>  	struct inode *inode = file_inode(file);
> @@ -2146,11 +2150,46 @@ int file_modified(struct file *file)
>  	ret = inode_needs_update_time(inode, &now);
>  	if (ret <= 0)
>  		return ret;
> +	if (flags & IOCB_NOWAIT)
> +		return -EAGAIN;
>  
>  	return __file_update_time(file, &now, ret);
>  }
> +
> +/**
> + * file_modified - handle mandated vfs changes when modifying a file
> + * @file: file that was modified
> + *
> + * When file has been modified ensure that special
> + * file privileges are removed and time settings are updated.
> + *
> + * Context: Caller must hold the file's inode lock.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int file_modified(struct file *file)
> +{
> +	return file_modified_flags(file, 0);
> +}
>  EXPORT_SYMBOL(file_modified);
>  
> +/**
> + * kiocb_modified - handle mandated vfs changes when modifying a file
> + * @iocb: iocb that was modified
> + *
> + * When file has been modified ensure that special
> + * file privileges are removed and time settings are updated.
> + *
> + * Context: Caller must hold the file's inode lock.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int kiocb_modified(struct kiocb *iocb)
> +{
> +	return file_modified_flags(iocb->ki_filp, iocb->ki_flags);
> +}
> +EXPORT_SYMBOL_GPL(kiocb_modified);
> +
>  int inode_needs_sync(struct inode *inode)
>  {
>  	if (IS_SYNC(inode))
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bdf1ce48a458..553e57ec3efa 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2392,6 +2392,7 @@ static inline void file_accessed(struct file *file)
>  }
>  
>  extern int file_modified(struct file *file);
> +int kiocb_modified(struct kiocb *iocb);
>  
>  int sync_inode_metadata(struct inode *inode, int wait);
>  
> -- 
> 2.30.2
>
Jan Kara June 2, 2022, 9:06 a.m. UTC | #2 
On Wed 01-06-22 14:01:36, Stefan Roesch wrote:
> This adds a file_modified_async() function to return -EAGAIN if the
> request either requires to remove privileges or needs to update the file
> modification time. This is required for async buffered writes, so the
> request gets handled in the io worker of io-uring.
> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

I've found one small bug here:

> diff --git a/fs/inode.c b/fs/inode.c
> index c44573a32c6a..4503bed063e7 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
...
> -int file_modified(struct file *file)
> +static int file_modified_flags(struct file *file, int flags)
>  {
>  	int ret;
>  	struct inode *inode = file_inode(file);

We need to use 'flags' for __file_remove_privs_flags() call in this patch.

								Honza
Stefan Roesch June 2, 2022, 9 p.m. UTC | #3 
On 6/2/22 2:06 AM, Jan Kara wrote:
> On Wed 01-06-22 14:01:36, Stefan Roesch wrote:
>> This adds a file_modified_async() function to return -EAGAIN if the
>> request either requires to remove privileges or needs to update the file
>> modification time. This is required for async buffered writes, so the
>> request gets handled in the io worker of io-uring.
>>
>> Signed-off-by: Stefan Roesch <shr@fb.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> I've found one small bug here:
> 
>> diff --git a/fs/inode.c b/fs/inode.c
>> index c44573a32c6a..4503bed063e7 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
> ...
>> -int file_modified(struct file *file)
>> +static int file_modified_flags(struct file *file, int flags)
>>  {
>>  	int ret;
>>  	struct inode *inode = file_inode(file);
> 
> We need to use 'flags' for __file_remove_privs_flags() call in this patch.
> 

I assume that you meant that the function should not be called _file_remove_privs(),
but instead file_remove_privs_flags(). Is that correct?

This would need to be changed in patch 8: "fs: add __remove_file_privs() with flags parameter"

> 								Honza
>
Jan Kara June 3, 2022, 10:12 a.m. UTC | #4 
On Thu 02-06-22 14:00:38, Stefan Roesch wrote:
> 
> 
> On 6/2/22 2:06 AM, Jan Kara wrote:
> > On Wed 01-06-22 14:01:36, Stefan Roesch wrote:
> >> This adds a file_modified_async() function to return -EAGAIN if the
> >> request either requires to remove privileges or needs to update the file
> >> modification time. This is required for async buffered writes, so the
> >> request gets handled in the io worker of io-uring.
> >>
> >> Signed-off-by: Stefan Roesch <shr@fb.com>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > I've found one small bug here:
> > 
> >> diff --git a/fs/inode.c b/fs/inode.c
> >> index c44573a32c6a..4503bed063e7 100644
> >> --- a/fs/inode.c
> >> +++ b/fs/inode.c
> > ...
> >> -int file_modified(struct file *file)
> >> +static int file_modified_flags(struct file *file, int flags)
> >>  {
> >>  	int ret;
> >>  	struct inode *inode = file_inode(file);
> > 
> > We need to use 'flags' for __file_remove_privs_flags() call in this patch.
> > 
> 
> I assume that you meant that the function should not be called _file_remove_privs(),
> but instead file_remove_privs_flags(). Is that correct?

No, I meant that patch 8 adds call __file_remove_privs(..., 0) to
file_modified() and this patch then forgets to update that call to
__file_remove_privs(..., flags) so that information propagates properly.

								Honza
Stefan Roesch June 6, 2022, 4:35 p.m. UTC | #5 
On 6/3/22 3:12 AM, Jan Kara wrote:
> On Thu 02-06-22 14:00:38, Stefan Roesch wrote:
>>
>>
>> On 6/2/22 2:06 AM, Jan Kara wrote:
>>> On Wed 01-06-22 14:01:36, Stefan Roesch wrote:
>>>> This adds a file_modified_async() function to return -EAGAIN if the
>>>> request either requires to remove privileges or needs to update the file
>>>> modification time. This is required for async buffered writes, so the
>>>> request gets handled in the io worker of io-uring.
>>>>
>>>> Signed-off-by: Stefan Roesch <shr@fb.com>
>>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>>
>>> I've found one small bug here:
>>>
>>>> diff --git a/fs/inode.c b/fs/inode.c
>>>> index c44573a32c6a..4503bed063e7 100644
>>>> --- a/fs/inode.c
>>>> +++ b/fs/inode.c
>>> ...
>>>> -int file_modified(struct file *file)
>>>> +static int file_modified_flags(struct file *file, int flags)
>>>>  {
>>>>  	int ret;
>>>>  	struct inode *inode = file_inode(file);
>>>
>>> We need to use 'flags' for __file_remove_privs_flags() call in this patch.
>>>
>>
>> I assume that you meant that the function should not be called _file_remove_privs(),
>> but instead file_remove_privs_flags(). Is that correct?
> 
> No, I meant that patch 8 adds call __file_remove_privs(..., 0) to
> file_modified() and this patch then forgets to update that call to
> __file_remove_privs(..., flags) so that information propagates properly.
> 

Thanks for the explanation, I made the change.
> 								Honza
>
diff mbox series

Patch

diff --git a/fs/inode.c b/fs/inode.c
index c44573a32c6a..4503bed063e7 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2116,17 +2116,21 @@  int file_update_time(struct file *file)
 EXPORT_SYMBOL(file_update_time);
 
 /**
- * file_modified - handle mandated vfs changes when modifying a file
+ * file_modified_flags - handle mandated vfs changes when modifying a file
  * @file: file that was modified
+ * @flags: kiocb flags
  *
  * When file has been modified ensure that special
  * file privileges are removed and time settings are updated.
  *
+ * If IOCB_NOWAIT is set, special file privileges will not be removed and
+ * time settings will not be updated. It will return -EAGAIN.
+ *
  * Context: Caller must hold the file's inode lock.
  *
  * Return: 0 on success, negative errno on failure.
  */
-int file_modified(struct file *file)
+static int file_modified_flags(struct file *file, int flags)
 {
 	int ret;
 	struct inode *inode = file_inode(file);
@@ -2146,11 +2150,46 @@  int file_modified(struct file *file)
 	ret = inode_needs_update_time(inode, &now);
 	if (ret <= 0)
 		return ret;
+	if (flags & IOCB_NOWAIT)
+		return -EAGAIN;
 
 	return __file_update_time(file, &now, ret);
 }
+
+/**
+ * file_modified - handle mandated vfs changes when modifying a file
+ * @file: file that was modified
+ *
+ * When file has been modified ensure that special
+ * file privileges are removed and time settings are updated.
+ *
+ * Context: Caller must hold the file's inode lock.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int file_modified(struct file *file)
+{
+	return file_modified_flags(file, 0);
+}
 EXPORT_SYMBOL(file_modified);
 
+/**
+ * kiocb_modified - handle mandated vfs changes when modifying a file
+ * @iocb: iocb that was modified
+ *
+ * When file has been modified ensure that special
+ * file privileges are removed and time settings are updated.
+ *
+ * Context: Caller must hold the file's inode lock.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int kiocb_modified(struct kiocb *iocb)
+{
+	return file_modified_flags(iocb->ki_filp, iocb->ki_flags);
+}
+EXPORT_SYMBOL_GPL(kiocb_modified);
+
 int inode_needs_sync(struct inode *inode)
 {
 	if (IS_SYNC(inode))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bdf1ce48a458..553e57ec3efa 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2392,6 +2392,7 @@  static inline void file_accessed(struct file *file)
 }
 
 extern int file_modified(struct file *file);
+int kiocb_modified(struct kiocb *iocb);
 
 int sync_inode_metadata(struct inode *inode, int wait);