On 5/6/22 2:29 AM, Dave Chinner wrote:
> On Mon, May 02, 2022 at 02:21:17PM -0700, Stefan Roesch wrote:
>>
>>
>> On 4/28/22 2:54 PM, Dave Chinner wrote:
>>> On Thu, Apr 28, 2022 at 12:58:59PM -0700, Stefan Roesch wrote:
>>>>
>>>>
>>>> On 4/26/22 3:56 PM, Dave Chinner wrote:
>>>>> On Tue, Apr 26, 2022 at 10:43:28AM -0700, Stefan Roesch wrote:
>>>>>> This adds the async buffered write support to XFS. For async buffered
>>>>>> write requests, the request will return -EAGAIN if the ilock cannot be
>>>>>> obtained immediately.
>>>>>>
>>>>>> Signed-off-by: Stefan Roesch <shr@xxxxxx>
>>>>>> ---
>>>>>> fs/xfs/xfs_file.c | 10 ++++++----
>>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>>>>>> index 6f9da1059e8b..49d54b939502 100644
>>>>>> --- a/fs/xfs/xfs_file.c
>>>>>> +++ b/fs/xfs/xfs_file.c
>>>>>> @@ -739,12 +739,14 @@ xfs_file_buffered_write(
>>>>>> bool cleared_space = false;
>>>>>> int iolock;
>>>>>>
>>>>>> - if (iocb->ki_flags & IOCB_NOWAIT)
>>>>>> - return -EOPNOTSUPP;
>>>>>> -
>>>>>> write_retry:
>>>>>> iolock = XFS_IOLOCK_EXCL;
>>>>>> - xfs_ilock(ip, iolock);
>>>>>> + if (iocb->ki_flags & IOCB_NOWAIT) {
>>>>>> + if (!xfs_ilock_nowait(ip, iolock))
>>>>>> + return -EAGAIN;
>>>>>> + } else {
>>>>>> + xfs_ilock(ip, iolock);
>>>>>> + }
>>>>>
>>>>> xfs_ilock_iocb().
>>>>>
>>>>
>>>> The helper xfs_ilock_iocb cannot be used as it hardcoded to use iocb->ki_filp to
>>>> get a pointer to the xfs_inode.
>>>
>>> And the problem with that is?
>>>
>>> I mean, look at what xfs_file_buffered_write() does to get the
>>> xfs_inode 10 lines about that change:
>>>
>>> xfs_file_buffered_write(
>>> struct kiocb *iocb,
>>> struct iov_iter *from)
>>> {
>>> struct file *file = iocb->ki_filp;
>>> struct address_space *mapping = file->f_mapping;
>>> struct inode *inode = mapping->host;
>>> struct xfs_inode *ip = XFS_I(inode);
>>>
>>> In what cases does file_inode(iocb->ki_filp) point to a different
>>> inode than iocb->ki_filp->f_mapping->host? The dio write path assumes
>>> that file_inode(iocb->ki_filp) is correct, as do both the buffered
>>> and dio read paths.
>>>
>>> What makes the buffered write path special in that
>>> file_inode(iocb->ki_filp) is not correctly set whilst
>>> iocb->ki_filp->f_mapping->host is?
>>>
>>
>> In the function xfs_file_buffered_write() the code calls the function
>> xfs_ilock(). The xfs_inode pointer that is passed in is iocb->ki_filp->f_mapping->host.
>> The one used in xfs_ilock_iocb is ki_filp->f_inode.
>>
>> After getting the lock, the code in xfs_file_buffered_write calls the
>> function xfs_buffered_write_iomap_begin(). In this function the code
>> calls xfs_ilock() for ki_filp->f_inode in exclusive mode.
>>
>> If I replace the first xfs_ilock() call with xfs_ilock_iocb(), then it looks
>> like I get a deadlock.
>>
>> Am I missing something?
>
> Yes. They take different locks. xfs_file_buffered_write() takes the
> IOLOCK, xfs_buffered_write_iomap_begin() takes the ILOCK....
>
Thanks for the clarification.
>> I can:
>> - replace the pointer to iocb with pointer to xfs_inode in the function xfs_ilock_iocb()
>> and also pass in the flags value as a parameter.
>> or
>> - create function xfs_ilock_inode(), which xfs_ilock_iocb() calls. The existing
>> calls will not need to change, only the xfs_ilock in xfs_file_buffered_write()
>> will use xfs_ilock_inode().
>
> You're making this way more complex than it needs to be. As I said:
>
>>> Regardless, if this is a problem, then just pass the XFS inode to
>>> xfs_ilock_iocb() and this is a moot point.
>
The function xfs_ilock_iocb() is expecting a pointer to the data structure kiocb, not
a pointer to xfs_inode. I don't see how that's possible without changing the signature
of xfs_ilock_iocb().
Do you want to invoke xfs_ilock_nowait() directly()?
> Cheers,
>
> Dave.
