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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
