Date: Mon, 12 Mar 2012 10:25:46 -0500
From: Ben Myers <bpm@sgi.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 4/8] xfs: log file size updates at I/O completion time
Message-ID: <20120312152546.GD7762@sgi.com>
References: <20120229095347.009884687@bombadil.infradead.org> <20120229095508.812609910@bombadil.infradead.org> <20120305200018.GN7762@sgi.com> <20120312132617.GA25944@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20120312132617.GA25944@infradead.org>
User-Agent: Mutt/1.5.18 (2008-05-17)

On Mon, Mar 12, 2012 at 09:26:17AM -0400, Christoph Hellwig wrote:
> On Mon, Mar 05, 2012 at 02:00:18PM -0600, Ben Myers wrote:
> > > -	if (iohead)
> > > +	if (iohead) {
> > > +		/*
> > > +		 * Reserve log space if we might write beyond the on-disk
> > > +		 * inode size.
> > > +		 */
> > > +		if (ioend->io_type != IO_UNWRITTEN &&
> > > +		    xfs_ioend_is_append(ioend)) {
> > 			^^^
> > 
> > I suggest that xfs_ioend_is_append should look at every ioend in the
> > chain in order to determine if an append is possible, not just the
> > first.  Note that xfs_submit_ioend_bio above is called for each ioend in
> > the chain.  You'd only see this on a system with a larger page size than
> > filesystem block size. 
> 
> It doesn't look at the first, it looks at the last one

Oh, it does look at the last one.  xfs_vm_writepage keeps a pointer to
the first one in iohead, but always calls xfs_ioend_is_append on the
last one.

>- I initially
> thought we might need to do it for all, but Dave convinced me otherwise.
> 
> I wish I'd still remember why exactly and should have written that down
> in a comment though.  I'll try to get back to it once I had a bit more
> sleep.

It looks like xfs_submit_ioend goes through the whole ioend list and
submit a bio for each, so you are already checking each one separately
in the completion handler.

> > In the situation where we are converting an unwritten extent we cancel
> > the preallocated transaction and call xfs_iomap_write_unwritten where
> > the inode core is logged with the updated size.  We were already
> > allocating an ioend here, so when you said 'To make this possible we
> > have to preallocate an ioend that allows deferring it here', did you
> > really mean to say that we're preallocating the transaction?  Maybe
> > there are just to many 'its' in the comment or I'm just dense.
> 
> I'll replace the comment with something that makes sense.

Thanks,
	Ben