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