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

[NeilBrown <neilb@suse.com>]

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

On Wed, Apr 12 2017, Jeff Layton wrote:

> 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...

From a technical perspective, it might be "simpler" but I contest "much
simpler".   I think it would be easy to put one wb_err_t per
nfs_open_context, if the former were designed well (which itself would
be easy).

From a political perspective, I doubt it would be simple.  NFS is the
way it is for a reason, and convincing an author that their reason is
not valid tends to be harder than most technical issues.
(looking to history...
the 'error' field was added to the nfs_open_context in
Commit: 6caf69feb23a ("NFSv2/v3/v4: Place NFS nfs_page shared data into a single structure    that hangs off filp->private_data. As a side effect, this also    cleans up the NFSv4 private file state info.")

in 2.6.12.  Prior to that file->f_error was used.
Prior to commit 9ffb8c3a1955 ("Import 2.2.3pre1") (which has no comment)
errors were ... interesting.  Look for nfs_check_error in
commit d9c0ffee4db7 ("Import 2.1.128") and notice the use of current->pid!!
All commits from the history.git tree.
)

It is quite possible for an NFS server to return different errors to
different users.  It might be odd, but it is possible.  Should an error
that affects one user pollute all other users?

Thanks,
NeilBrown

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