From: NeilBrown <neilb@suse.com>
To: Jeff Layton <jlayton@redhat.com>, linux-fsdevel@vger.kernel.org
Date: Tue, 18 Apr 2017 08:53:29 +1000
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 07/17] fs: new infrastructure for writeback error handling and reporting
In-Reply-To: <1492038117.19286.6.camel@redhat.com>
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> <1492038117.19286.6.camel@redhat.com>
Message-ID: <87y3uytzme.fsf@notabene.neil.brown.name>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4060
Lines: 122

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

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:
>>=20
>>=20
>> > +void __filemap_set_wb_error(struct address_space *mapping, int err)
>>=20
>> I was really hoping that this would be
>>=20
>>   void __set_wb_error(wb_err_t *wb_err, int err)
>>=20
>> so
>>=20
>> Then nfs_context_set_write_error could become
>>=20
>> static void nfs_context_set_write_error(struct nfs_open_context *ctx, in=
t error)
>> {
>> 	__set_wb_error(&ctx->wb_err, error);
>> }
>>=20
>> and filemap_set_sb_error() would be:
>>=20
>> static inline void filemap_set_wb_error(struct address_space *mapping, i=
nt err)
>> {
>> 	/* Optimize for the common case of no error */
>> 	if (unlikely(err))
>> 		__set_wb_error(&mapping->f_wb_err, err);
>> }
>>=20
>> Similarly we would have
>>   wb_err_t sample_wb_error(wb_err_t *wb_err)
>>   {
>>    ...
>>   }
>>=20
>> and
>>=20
>> wb_err_t filemap_sample_wb_error(struct address_space *mapping)
>> {
>>   return sample_wb_error(&mapping->f_wb_err);
>> }
>>=20
>> so nfs_file_fsync_commit() could have
>>   ret =3D sample_wb_error(&ctx->wb_err);
>> in place of
>> 	ret =3D xchg(&ctx->error, 0);
>>=20
>> int filemap_report_wb_error(struct file *file)
>>=20
>> would become
>>=20
>> int filemap_report_wb_error(struct file *file, wb_err_t *err)
>>=20
>> or something.
>>=20
>> 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).
>>=20
>>=20
>
> 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=20a 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=20a 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 s=
ingle structure    that hangs off filp->private_data. As a side effect, thi=
s 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

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAlj1R2kACgkQOeye3VZi
gbk94g//UqyhRu8EqbeUc59rgovkW6PEvBFiL6bb27W9xWWoZHd0COfu+n9l3ikk
vfrkENGFbFYhTz4DAYY6VpfQ+M0TJu1ovyPih2TLsZio5E7QWBI1y3B8Xvtte+6y
WqLpL6AP8CLqGrTzQbwbqhRHmtq8Cu4nglQFyDyDkqP8Pv1Kcy2H3bmlFhKs0tnK
00eoT6XUcQwdfd2WcpKMUGsxpHgWh1TlplZ+NHT0Eu5iRP+aP8Pc5/heYtc+kSrb
c1JXRRoQvAjyna4MUcbQsimbEnjxXVDiUYQkJvAVXMklptifi3oftN/tVk+62Pgg
ywvMJNmFwacdFZCKbu1xAB4OyyOf9DxN4d+bBy28FoGPSf16Ma37DOW9so0EhXMU
xpmmM7L81/U/Xe6fy92jg46htdhC8SoiJv86WJ8fRTeC4gaEvfH0WtUT7Yu5KroM
LQd6u2co+eRqE9Jaua+7atCNfNPb16LvhHIUCdgRSitH9cLoPyC3nG/Vq6S9BOH/
yaUBztRVECgMohgGNclhrGulomthXBcItbFKe/cwkUmCDza6Q+NjwHI5pnjd+xTa
/KjWAGrKEseYGrer5y9OqFxdajoXisNuj4aUhFSsbgLnr5QzI1VkiPC9afhltKIo
BUAcrgjK8xEcAA18DK4bnk5M7cNN0nFCLyxnBv7Vu0S9qT/wS/8=
=3YxO
-----END PGP SIGNATURE-----
--=-=-=--