xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/8] xfs: log file size updates at I/O completion time
From: Ben Myers <bpm@xxxxxxx>
Date: Mon, 12 Mar 2012 10:25:46 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120312132617.GA25944@xxxxxxxxxxxxx>
References: <20120229095347.009884687@xxxxxxxxxxxxxxxxxxxxxx> <20120229095508.812609910@xxxxxxxxxxxxxxxxxxxxxx> <20120305200018.GN7762@xxxxxxx> <20120312132617.GA25944@xxxxxxxxxxxxx>
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

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