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@xxxxxxxxxx>
