xfs
[Top] [All Lists]

Re: [PATCH 1/7] xfs: ensure sync write errors are returned

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 1/7] xfs: ensure sync write errors are returned
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 17 Dec 2010 17:43:29 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1292532649.2457.54.camel@doink>
References: <1292376208-16282-1-git-send-email-david@xxxxxxxxxxxxx> <1292376208-16282-2-git-send-email-david@xxxxxxxxxxxxx> <1292532649.2457.54.camel@doink>
User-agent: Mutt/1.5.20 (2009-06-14)
On Thu, Dec 16, 2010 at 02:50:49PM -0600, Alex Elder wrote:
> On Wed, 2010-12-15 at 12:23 +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > xfs_file_aio_write() only returns the error from synchronous
> > flushing of the data and inode if error == 0. At the point where
> > error is being checked, it is guaranteed to be > 0. Therefore any
> 
> Actually, we have this above the affected code:
>         ...
>         error = -ret;   
>         if (ret <= 0)
>                 goto out_unlock_internal;
> 
> So ret must be positive, therefore error is negative
> (the negative of the number of bytes written).  In
> other words, we enter this block without having seen
> an error.
> 
> The return at the end of the function is:
>       return -error;
> 
> And by the convoluted logic here, that means that the
> value of error should be a negative byte count if
> successful, or a positive errno otherwise.

Oh, right, yeah, I screwed that up, didn't I?

> And since filemap_write_and_wait_range() returns a
> negative errno, your fix doesn't look right to me.

I will fix it up.

> The existing code is wrong and should be fixed, but a
> better fix might make the meaning of the variable "error"
> a little less weird.

The real problem is that xfs functions return positive errors, and
the linux functions return negative errors.  It would be much less
of a hassle if we fixed that problem...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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