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

[Christoph Hellwig]

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 at lst.de>
> > 
> > ---
> ....
> > @@ -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.