Re: [PATCH v2 08/17] fs: retrofit old error reporting API onto new infrastructure

From: NeilBrown <neilb@suse.com>
To: Jeff Layton <jlayton@redhat.com>, linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
	tytso@mit.edu, jack@suse.cz, willy@infradead.org,
	viro@zeniv.linux.org.uk
Subject: Re: [PATCH v2 08/17] fs: retrofit old error reporting API onto new infrastructure
Date: Mon, 24 Apr 2017 08:38:54 +1000	[thread overview]
Message-ID: <87pog2rbpd.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <1492778818.7308.8.camel@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 3508 bytes --]

On Fri, Apr 21 2017, Jeff Layton wrote:

> On Tue, 2017-04-18 at 08:56 +1000, NeilBrown wrote:
>> On Wed, Apr 12 2017, Jeff Layton wrote:
>> 
>> > On Thu, 2017-04-13 at 08:14 +1000, NeilBrown wrote:
>> > > 
>> > > I suspect that the filemap_check_wb_error() will need to be moved
>> > > into some parent of the current call site, which is essentially what you
>> > > suggest below.  It would be nice if we could do that first, rather than
>> > > having the current rather odd code.  But maybe this way is an easier
>> > > transition.  It isn't obviously wrong, it just isn't obviously right
>> > > either.
>> > > 
>> > 
>> > Yeah. It's just such a daunting task to have to change so much of the
>> > existing code. I'm looking for ways to make this simpler.
>> > 
>> > I think it probably is reasonable for filemap_write_and_wait* to just
>> > sample it as early as possible in those functions. filemap_fdatawait is
>> > the real questionable one, as you may have already had some writebacks
>> > complete with errors.
>> > 
>> > In any case, my thinking was that the old code is not obviously correct
>> > either, so while this shortens the "error capture window" on these
>> > calls, it seems like a reasonable place to start improving things.
>> 
>> I agree.  It wouldn't hurt to add a note to this effect in the patch
>> comment so that people understand that the code isn't seen to be
>> "correct" but only "no worse" with clear direction on what sort of
>> improvement might be appropriate.
>> 
>
> I've got a cleaned-up set that is getting close to ready for
> reposting. Before I do though, I think there is another option here
> that's worth discussing.
>
> We could store a second wb_err_t (aka errseq_t in the new set) in the
> mapping that would would basically act as a "cursor" for these cases.
> filemap_check_errors would need to do something like 
> filemap_report_wb_error, but it would swap the value into the mapping's
> cursor instead of dealing with the one in struct file.
>
> I don't really like adding yet another field here, but the struct
> address_space definition has this:
>
>     __attribute__((aligned(sizeof(long))));
>
> Adding the wb_err field means that we end up growing the struct by 8
> bytes on x86_64 anyway. Adding another 4 bytes would just consume the
> pad, so it wouldn't cost anything there. YMMV on other arches of
> course.
>
> That's also not perfectly like what we have with AS_EIO/AS_ENOSPC
> flags, but is probably close enough not to matter.
>
> So...this would let us limp along for even longer with the model of
> reporting since last check. I'm not sure that's a good thing though. A
> long term goal here is to have kernel code that's dealing with
> writeback be more deliberate about the point from which it's checking
> errors, and this doesn't help promote that.

I think this question needs some input from filesystem developers who
might be affected by the answer.

My preference is to not add this field.  I think we would eventually
want to remove it again, and it is easier to ensure it doesn't stay
forever if it is never added.
The version without this field isn't (I think) too bad, but maybe it is
bad enough to motivate fs developers to create a better solution in each
individual case.

If some filesystem developer says they don't like that sort of social
engineering, or objects for any other reason, I will bow to the superior
stake they hold.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]