On Fri, Oct 12, 2012 at 09:11:37AM +1100, Dave Chinner wrote:
> On Thu, Oct 11, 2012 at 05:38:02PM -0400, Christoph Hellwig wrote:
> > > index 917e121..dee14eb 100644
> > > --- a/fs/xfs/xfs_fsops.c
> > > +++ b/fs/xfs/xfs_fsops.c
> > > @@ -149,6 +149,11 @@ xfs_growfs_data_private(
> > > XFS_FSS_TO_BB(mp, 1), 0, NULL);
> > > if (!bp)
> > > return EIO;
> > > + if (bp->b_error) {
> > > + int error = bp->b_error;
> > > + xfs_buf_relse(bp);
> > > + return error;
> > > + }
> > > xfs_buf_relse(bp);
> >
> > > + if (bp->b_error) {
> > > + error = bp->b_error;
> > > + if (loud)
> > > + xfs_warn(mp, "SB validate failed");
> > > + goto release_buf;
> > > + }
> >
> > > + if (bp->b_error) {
> > > + error = bp->b_error;
> > > + xfs_buf_relse(bp);
> > > + return error;
> > > + }
> >
> > > + if (!bp || bp->b_error) {
> > > xfs_warn(mp, "realtime device size check failed");
> > > + if (bp)
> > > + xfs_buf_relse(bp);
> > > return EIO;
> > > }
> > > xfs_buf_relse(bp);
> >
> > It seems like all these callers would be a lot cleaner if we'd just
> > return the error as the return value, and a buffer as an indirect
> > pointer if and only if the read succeeded.
>
> The number of callers is relatively small, and the knock-on effect
> through the subsequent patches isn't that bad, so changing the
> interface is probably the right thing to do here. I'll rework this
> patch appropriately.
Then again, maybe I won't. I just remembered the main reason for
returning a buffer with an error set on it. That is that there are
circumstances in which we might want to attempt repair of a buffer
that failed verification, and we cannot do that without returning
the buffer.
So even if we return the buffer as an indirect pointer, we'd still
need to return it when particular types of errors are detected on
the buffer (i.e. EFSCORRUPTED from the verifier). Hence the error
handling in xfs_buf_read_uncached() would become more complex and
difficult to get right (e.g. different EIO vs EFSCORRUPTED vs ...
handling) and it wouldn't simplify the error handling at the call
site, either.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
