xfs
[Top] [All Lists]

Re: [PATCH 4/5] xfs: log file size updates at I/O completion time

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/5] xfs: log file size updates at I/O completion time
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sun, 19 Feb 2012 16:37:05 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20120216071028.GD14132@dastard>
References: <20120207181037.745771452@xxxxxxxxxxxxxxxxxxxxxx> <20120207181155.511135108@xxxxxxxxxxxxxxxxxxxxxx> <20120216071028.GD14132@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Feb 16, 2012 at 06:10:28PM +1100, Dave Chinner wrote:
> On Tue, Feb 07, 2012 at 01:10:41PM -0500, Christoph Hellwig wrote:
> > Do not use unlogged metadata updates and the VFS dirty bit for updating
> > the file size after writeback.  In addition to causing various problems
> > with updates getting delayed for far too log this also drags in the
>                                          long
> 
> > unscalable VFS dirty tracking, and is one of the few remaining unlogged
> > metadata updates.
> > 
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > 
> > ---
> ....
> > @@ -173,18 +214,26 @@ xfs_end_io(
> >      * range to normal written extens after the data I/O has finished.
> >      */
> >     if (ioend->io_type == IO_UNWRITTEN) {
> > +           if (ioend->io_append_trans) {
> > +                   ASSERT(ioend->io_isdirect);
> > +
> > +                   current_set_flags_nested(
> > +                           &ioend->io_append_trans->t_pflags, PF_FSTRANS);
> > +                   xfs_trans_cancel(ioend->io_append_trans, 0);
> > +           }
> > +
> >             error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
> >                                              ioend->io_size);
> >             if (error) {
> >                     ioend->io_error = -error;
> >                     goto done;
> >             }
> > +   } else if (ioend->io_append_trans) {
> > +           error = xfs_setfilesize(ioend);
> > +           if (error)
> > +                   ioend->io_error = error;
> 
> That looks like it should be:
> 
>                       ioend->io_error = -error;

It should to be consistant with the other users.  But in practice it
doesn't matter given that we effectively use the field as a boolean flag
for now. 

> >     ioend->io_iocb = NULL;
> >     ioend->io_result = 0;
> > +   ioend->io_append_trans = NULL;
> 
> This is starting to look like it should memset the ioend to zero
> after allocation rather than all these individual "initialise to
> zero" lines.

I'll leave that out for now - in the direct I/O code (which also calls
this routine) doing the reverse on a somewhat larger structured proved
to give large measureable performance gains.

> > +           size_t size = iov_length(iov, nr_segs);
> > +
> > +           iocb->private = ioend = xfs_alloc_ioend(inode, IO_DIRECT);
> > +           if (offset + size > XFS_I(inode)->i_d.di_size) {
> > +                   ret = xfs_setfilesize_trans_alloc(ioend);
> > +                   if (ret)
> > +                           goto destroy_ioend;
> > +                   ioend->io_isdirect = 1;
> > +           }
> 
> This is a bit messy, but necessary to handle racing IOs and
> unwritten extent conversion.. Can you add a comment explining
> why it is necessary so we don't forget in future?

I probably should, especially given that I have already forgotten how
messy exactly the direct I/O code is in that area.

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