xfs
[Top] [All Lists]

Re: [PATCH 1/5] xfs: DIO requires an ioend for writes

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 1/5] xfs: DIO requires an ioend for writes
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Sat, 11 Apr 2015 08:24:25 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150410202137.GA2846@xxxxxxxxxxxxxx>
References: <1428673080-23052-1-git-send-email-david@xxxxxxxxxxxxx> <1428673080-23052-2-git-send-email-david@xxxxxxxxxxxxx> <20150410202137.GA2846@xxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Apr 10, 2015 at 04:21:37PM -0400, Brian Foster wrote:
> On Fri, Apr 10, 2015 at 11:37:56PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Right now unwritten extent conversion information is passed by
> > making the end IO private data non-null, which does not enable us to
> > pass any information from submission context to completion context,
> > which we need to use the standard IO completion paths.
> > 
> > Allocate an ioend in block allocation for direct IO and attach it to
> > the mapping buffer used during direct IO block allocation. Factor
> > the mapping code to make it obvious that this is happening only for
> > direct IO writes, and and place the mapping info and IO type
> > directly into the ioend for use in completion context.
> > 
> > The completion is changed to check the ioend type to determine if
> > unwritten extent completion is necessary or not.
> > 
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> >  fs/xfs/xfs_aops.c | 79 
> > ++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 61 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > index 3a9b7a1..d95a42b 100644
> > --- a/fs/xfs/xfs_aops.c
> > +++ b/fs/xfs/xfs_aops.c
> > @@ -1233,6 +1233,57 @@ xfs_vm_releasepage(
> >     return try_to_free_buffers(page);
> >  }
> >  
> > +static void
> > +xfs_get_blocks_map_buffer(
> > +   struct inode            *inode,
> > +   struct buffer_head      *bh_result,
> > +   int                     create,
> > +   int                     direct,
> > +   struct xfs_bmbt_irec    *imap,
> > +   xfs_off_t               offset,
> > +   ssize_t                 size)
> > +{
> > +   struct xfs_ioend        *ioend;
> > +   int                     type;
> > +
> > +   if (!create) {
> > +           /*
> > +            * Unwritten extents do not report a disk address for
> > +            * the read case (treat as if we're reading into a hole).
> > +            */
> > +           if (!ISUNWRITTEN(imap))
> > +                   xfs_map_buffer(inode, bh_result, imap, offset);
> > +           return;
> > +   }
> 
> This logic was kind of ugly to begin with, but I think the refactoring
> exposes it further. There's rather twisty logic here just for a case

Yup, I isolated it first to make it easy to change, not necessarily
easier to read ;)

....

> So if we pull some of the bits from xfs_get_blocks_map_buffer() back up,
> I end up with something like the the following here. Compile tested
> only, but illustrates the point:
> 
>         /*
>          * Map the buffer as long as we have physical blocks and this isn't a
>          * read of an unwritten extent. Treat reads into unwritten extents as
>          * holes and thus do not return a mapping.
>          */
>         if (imap.br_startblock != HOLESTARTBLOCK &&
>             imap.br_startblock != DELAYSTARTBLOCK &&
>             (create || !ISUNWRITTEN(&imap))) {
>                 xfs_map_buffer(inode, bh_result, &imap, offset);
>               /* unwritten implies create due to check above */
>                 if (ISUNWRITTEN(&imap))
>                         set_buffer_unwritten(bh_result);
>                 /* direct writes have a special mapping */
>                 if (create && direct) {
>                         error = xfs_map_direct(inode, bh_result, &imap, 
> offset);
>                         if (error)
>                                 return error;
>                 }
>         }
> 
> I renamed the helper to xfs_map_direct(), killed everything therein up
> through the !direct check and killed both the create and direct params.
> Thoughts?

Yeah, that looks neater; I'll split and rework it in a similar
manner to this. Thanks!

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>