On Thu, Feb 04, 2016 at 08:14:55AM +0100, Christoph Hellwig wrote:
> On Wed, Feb 03, 2016 at 11:55:31AM -0800, Darrick J. Wong wrote:
> > This will have the effect of a later error superseding an earlier error. I'm
> > under the impression that code should generally preserve the first error, since
> > some side effect of that probably caused the rest of the errors.
> >
> > That said, my guess is that 95% of the time err is set, retval and err will
> > both be -EIO anyway. I'm not particularly passionate about whether or not we
> > preserve the first error code.
>
> This leaves the option to the file system to pass the value through
> or not. Note that ret before the call will usually have the positive
> number of bytes written, so checking if it's 'set' wouldn't be enough
> even if adding some special casing in the callers.
Ok, I can live with that.
> > > +static int ext4_end_io_dio(struct kiocb *iocb, loff_t offset,
> > > ssize_t size, void *private)
> > > {
> > > ext4_io_end_t *io_end = iocb->private;
> > >
> > > + if (size <= 0)
> > > + return 0;
> >
> > This leaks the ext4_io_end_t, if there was one. Granted, that only happens
> > during an AIO DIO to an unwritten extent, but in any case I suggest removing
> > this hunk and...
>
> It's the same behavior as before - and if you look at ext4_ext_direct_IO
> it seems to expect this and works around it.
Gotcha. That's right, so I'll stop worrying about these. :)
--D
>
> > > + if (bytes <= 0)
> > > + return 0;
> > > +
> >
> > I suspect we still need to unlock the mutexes later on in this function.
> >
> > > /* this io's submitter should not have unlocked this before we could */
> > > BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
> > >
> > > @@ -644,6 +647,8 @@ static void ocfs2_dio_end_io(struct kiocb *iocb,
> > > level = ocfs2_iocb_rw_locked_level(iocb);
> > > ocfs2_rw_unlock(inode, level);
> > > }
> >
> > Do we need to still have an accurate value for bytes the conditional above
> > even if the IO errored out?
>
> Again, no changes to the old behavior. ocfs has some magic stuffed
> in iocb->private to deal with the locked state of an iocb, and while
> I don't fully understand it I suspect it's to handle the existing
> odd ->end_io calling conventions. Cleaning this up would be nice,
> but let's keep that a separate patch.
>
> > > struct kiocb *iocb,
> > > loff_t offset,
> > > @@ -1655,15 +1655,19 @@ xfs_end_io_direct_write(
> > > struct inode *inode = file_inode(iocb->ki_filp);
> > > struct xfs_ioend *ioend = private;
> > >
> > > + if (size <= 0)
> > > + return 0;
> >
> > Same thing here, I think we can end up leaking the ioend.
>
> This keeps the existing behavior. But either way, at least for
> XFS all this will be properly fixed in the next patch anyway.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
