Re: [PATCH v8 06/19] block, fs: Propagate write hints to the block device inode

[Kanchan Joshi <joshi.k@xxxxxxxxxxx>]

On 1/4/2024 4:39 AM, Bart Van Assche wrote:
> On 1/3/24 01:02, Christoph Hellwig wrote:
>> So you can use file->f_mapping->inode as I said in my previous mail.
> 
> Since struct address_space does not have a member with the name "inode",
> I assume that you meant "host" instead of "inode"? If so, how about
> modifying patch 06 of this series as shown below? With the patch below
> my tests still pass.
> 
> Thanks,
> 
> Bart.
> 
> 
> ---
>   block/fops.c       | 11 -----------
>   fs/fcntl.c         | 15 +++++++++++----
>   include/linux/fs.h |  1 -
>   3 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/block/fops.c b/block/fops.c
> index 138b388b5cb1..787ce52bc2c6 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -620,16 +620,6 @@ static int blkdev_release(struct inode *inode, 
> struct file *filp)
>       return 0;
>   }
> 
> -static void blkdev_apply_whint(struct file *file, enum rw_hint hint)
> -{
> -    struct bdev_handle *handle = file->private_data;
> -    struct inode *bd_inode = handle->bdev->bd_inode;
> -
> -    inode_lock(bd_inode);
> -    bd_inode->i_write_hint = hint;
> -    inode_unlock(bd_inode);
> -}
> -
>   static ssize_t
>   blkdev_direct_write(struct kiocb *iocb, struct iov_iter *from)
>   {
> @@ -864,7 +854,6 @@ const struct file_operations def_blk_fops = {
>       .splice_read    = filemap_splice_read,
>       .splice_write    = iter_file_splice_write,
>       .fallocate    = blkdev_fallocate,
> -    .apply_whint    = blkdev_apply_whint,
>   };
> 
>   static __init int blkdev_init(void)
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 18407bf5bb9b..cfb52c3a4577 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -306,7 +306,6 @@ static long fcntl_get_rw_hint(struct file *file, 
> unsigned int cmd,
>   static long fcntl_set_rw_hint(struct file *file, unsigned int cmd,
>                     unsigned long arg)
>   {
> -    void (*apply_whint)(struct file *, enum rw_hint);
>       struct inode *inode = file_inode(file);
>       u64 __user *argp = (u64 __user *)arg;
>       u64 hint;
> @@ -318,11 +317,19 @@ static long fcntl_set_rw_hint(struct file *file, 
> unsigned int cmd,
> 
>       inode_lock(inode);
>       inode->i_write_hint = hint;
> -    apply_whint = inode->i_fop->apply_whint;
> -    if (apply_whint)
> -        apply_whint(file, hint);
>       inode_unlock(inode);
> 
> +    /*
> +     * file->f_mapping->host may differ from inode. As an example,
> +     * blkdev_open() modifies file->f_mapping.
> +     */
> +    if (file->f_mapping->host != inode) {
> +        inode = file->f_mapping->host;
> +        inode_lock(inode);
> +        inode->i_write_hint = hint;
> +        inode_unlock(inode);
> +    }
> +

Are you considering to change this so that hint is set only on one inode 
(and not on two)?
IOW, should not this fragment be like below:

--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -306,7 +306,6 @@ static long fcntl_get_rw_hint(struct file *file, 
unsigned int cmd,
  static long fcntl_set_rw_hint(struct file *file, unsigned int cmd,
                               unsigned long arg)
  {
-       void (*apply_whint)(struct file *, enum rw_hint);
         struct inode *inode = file_inode(file);
         u64 __user *argp = (u64 __user *)arg;
         u64 hint;
@@ -316,11 +315,15 @@ static long fcntl_set_rw_hint(struct file *file, 
unsigned int cmd,
         if (!rw_hint_valid(hint))
                 return -EINVAL;

+       /*
+        * file->f_mapping->host may differ from inode. As an example
+        * blkdev_open() modifies file->f_mapping
+        */
+       if (file->f_mapping->host != inode)
+               inode = file->f_mapping->host;
+
         inode_lock(inode);
         inode->i_write_hint = hint;
-       apply_whint = inode->i_fop->apply_whint;
-       if (apply_whint)
-               apply_whint(file, hint);
         inode_unlock(inode);