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?
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().
> Regardless, if this is a problem, then just pass the XFS inode to
> xfs_ilock_iocb() and this is a moot point.
>
>> However here we need to use iocb->ki_filp->f_mapping->host.
>> I'll split off new helper for this in the next version of the patch.
>
> We don't need a new helper here, either.
>
> Cheers,
>
> Dave.
