linux-kernel - Re: [PATCH v2 07/17] fs: new infrastructure for writeback error handling and reporting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1492038117.19286.6.camel@redhat.com>
Date:   Wed, 12 Apr 2017 19:01:57 -0400
From:   Jeff Layton <jlayton@...hat.com>
To:     NeilBrown <neilb@...e.com>, linux-fsdevel@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
        tytso@....edu, jack@...e.cz, willy@...radead.org,
        viro@...iv.linux.org.uk
Subject: Re: [PATCH v2 07/17] fs: new infrastructure for writeback error
 handling and reporting

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@...hat.com>