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