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

Kanchan Joshi
2024-01-19T13:56:37Z

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.