Message-ID: <1492038117.19286.6.camel@redhat.com>
Subject: Re: [PATCH v2 07/17] fs: new infrastructure for writeback error
 handling and reporting
From: Jeff Layton <jlayton@redhat.com>
To: NeilBrown <neilb@suse.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
Date: Wed, 12 Apr 2017 19:01:57 -0400
In-Reply-To: <87inm9uw8b.fsf@notabene.neil.brown.name>
References: <20170412120614.6111-1-jlayton@redhat.com>
         <20170412120614.6111-8-jlayton@redhat.com>
         <1492022521.2937.18.camel@redhat.com>
         <87inm9uw8b.fsf@notabene.neil.brown.name>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2321
Lines: 90

On Thu, 2017-04-13 at 07:55 +1000, NeilBrown wrote:
> On Wed, Apr 12 2017, Jeff Layton wrote:
> 
> 
> > +void __filemap_set_wb_error(struct address_space *mapping, int err)
> 
> I was really hoping that this would be
> 
>   void __set_wb_error(wb_err_t *wb_err, int err)
> 
> so
> 
> Then nfs_context_set_write_error could become
> 
> static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
> {
> 	__set_wb_error(&ctx->wb_err, error);
> }
> 
> and filemap_set_sb_error() would be:
> 
> static inline void filemap_set_wb_error(struct address_space *mapping, int err)
> {
> 	/* Optimize for the common case of no error */
> 	if (unlikely(err))
> 		__set_wb_error(&mapping->f_wb_err, err);
> }
> 
> Similarly we would have
>   wb_err_t sample_wb_error(wb_err_t *wb_err)
>   {
>    ...
>   }
> 
> and
> 
> wb_err_t filemap_sample_wb_error(struct address_space *mapping)
> {
>   return sample_wb_error(&mapping->f_wb_err);
> }
> 
> so nfs_file_fsync_commit() could have
>   ret = sample_wb_error(&ctx->wb_err);
> in place of
> 	ret = xchg(&ctx->error, 0);
> 
> int filemap_report_wb_error(struct file *file)
> 
> would become
> 
> int filemap_report_wb_error(struct file *file, wb_err_t *err)
> 
> or something.
> 
> The address space is just one (obvious) place where the wb error can be
> stored.  The filesystem might have a different place with finer
> granularity (nfs already does).
> 
> 

I think it'd be much simpler to adapt NFS over to use the new
infrastructure (I have a draft patch for that already). You'd lose the
ability to track a different error for each nfs_open_context, but I'm
not sure how valuable that is anyway. I'll need to think about that
one...

> > +wb_err_t filemap_sample_wb_error(struct address_space *mapping)
> > +{
> > +	wb_err_t old = READ_ONCE(mapping->wb_err);
> > +	wb_err_t new = old;
> > +
> > +	/*
> > +	 * For the common case of no errors ever having been set, we can skip
> > +	 * marking the SEEN bit. Once an error has been set, the value will
> > +	 * never go back to zero.
> > +	 */
> > +	if (old != 0) {
> > +		new |= WB_ERR_SEEN;
> > +		if (old != new)
> > +			cmpxchg(&mapping->wb_err, old, new);
> > +	}
> > +	return new;
> > +}
> 
> I do like how the use of cmpxchg work out here - no looping!
> 

Me too. :)
-- 
Jeff Layton <jlayton@redhat.com>