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

[Kanchan Joshi <joshi.k@samsung.com>]

On 1/19/2024 7:26 PM, Kanchan Joshi wrote:
> On 1/19/2024 12:24 AM, Bart Van Assche wrote:
>> On 1/18/24 10:51, Kanchan Joshi wrote:
>>> 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);
>>
>> I think the above proposal would introduce a bug: it would break the
>> F_GET_RW_HINT implementation.
> 
> Right. I expected to keep the exact change in GET, too, but that will
> not be free from the side-effect.
> The buffered-write path (block_write_full_page) picks the hint from one
> inode, and the direct-write path (__blkdev_direct_IO_simple) picks the
> hint from a different inode.
> So, updating both seems needed here.

I stand corrected. It's possible to do away with two updates.
The direct-io code (patch 8) should rather be changed to pick the hint 
from bdev inode (and not from file inode).
With that change, this patch only need to set the hint into only one 
inode (bdev one). What do you think?