From: NeilBrown <neilb@suse.com>
To: Jeff Layton <jlayton@redhat.com>, linux-fsdevel@vger.kernel.org
Date: Thu, 13 Apr 2017 08:14:24 +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 08/17] fs: retrofit old error reporting API onto new infrastructure
In-Reply-To: <20170412120614.6111-9-jlayton@redhat.com>
References: <20170412120614.6111-1-jlayton@redhat.com> <20170412120614.6111-9-jlayton@redhat.com>
Message-ID: <87fuhduvcv.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: 4983
Lines: 121

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

On Wed, Apr 12 2017, Jeff Layton wrote:

> Now that we have a better way to store and report errors that occur
> during writeback, we need to convert the existing codebase to use it. We
> could just adapt all of the filesystem code and related infrastructure
> to the new API, but that's a lot of churn.
>
> When it comes to setting errors in the mapping, filemap_set_wb_error is
> a drop-in replacement for mapping_set_error. Turn that function into a
> simple wrapper around the new one.
>
> Because we want to ensure that writeback errors are always reported at
> fsync time, inject filemap_report_wb_error calls much closer to the
> syscall boundary. For fsync calls (and things like the nfsd equivalent),
> we either return the error that the fsync operation returns, or the one
> returned by filemap_report_wb_error. In both cases, we advance the
> file->f_wb_err to the latest value.
>
> The final piece of the puzzle is what to do about filemap_check_errors
> calls that are being called directly or via filemap_* functions. Here,
> we must take a little "creative license".
>
> Since we now handle advancing the file->f_wb_err value at the generic
> filesystem layer, we no longer need those callers to do anything like
> that, or clear errors out of the mapping. A lot of the existing codebase
> relies on being getting an error back from those functions when there is
> a writeback problem, so we do still want to have them report writeback
> errors somehow.
>
> When reporting writeback errors, we will always report errors that have
> occurred since a particular point in time. With the old writeback error
> reporting, the time we used was "since it was last tested/cleared" which
> is entirely arbitrary and potentially racy. Now, we can at least report
> the latest error that has occurred since an arbitrary point in time
> (represented as a sampled wb_err_t value).
>
> In the case where we don't have a struct file to work with, this patch
> just has the wrappers sample the mapping->wb_err value, and use that as
> an arbitrary point from which to check for errors.
>
> That's probably not ideal, particularly in the case of something like
> filemap_fdatawait, but I'm not sure it's any worse than what we
> already have, and this gives us a basis from which to work.

This aspect of the patch looked rather odd -- sampling a wb_err_t at
some arbitrary time, and then comparing it later.  So that for going to
the trouble of explaining it.

I suspect that the filemap_check_wb_error() will need to be moved
into some parent of the current call site, which is essentially what you
suggest below.  It would be nice if we could do that first, rather than
having the current rather odd code.  But maybe this way is an easier
transition.  It isn't obviously wrong, it just isn't obviously right
either.

>  int vfs_fsync_range(struct file *file, loff_t start, loff_t end, int dat=
async)
>  {
> +	int ret, ret2;
>  	struct inode *inode =3D file->f_mapping->host;
>=20=20
>  	if (!file->f_op->fsync)
> @@ -192,7 +193,9 @@ int vfs_fsync_range(struct file *file, loff_t start, =
loff_t end, int datasync)
>  		spin_unlock(&inode->i_lock);
>  		mark_inode_dirty_sync(inode);
>  	}
> -	return call_fsync(file, start, end, datasync);
> +	ret =3D call_fsync(file, start, end, datasync);
> +	ret2 =3D filemap_report_wb_error(file);
> +	return ret ? : ret2;
>  }
>  EXPORT_SYMBOL(vfs_fsync_range);
>=20=20
....

>  static int shm_fsync(struct file *file, loff_t start, loff_t end, int da=
tasync)
>  {
> +	int ret, ret2;
>  	struct shm_file_data *sfd =3D shm_file_data(file);
>=20=20
>  	if (!sfd->file->f_op->fsync)
>  		return -EINVAL;
> -	return call_fsync(sfd->file, start, end, datasync);
> +	ret =3D call_fsync(sfd->file, start, end, datasync);
> +	ret2 =3D filemap_report_wb_error(file);
> +	return ret ? : ret2;
>  }

These are the only two places that call_fsync() is called.
Did you consider moving the filemap_report_wb_error() call into
call_fsync() ??

Thanks,
NeilBrown

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

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

iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAljupsAACgkQOeye3VZi
gbnVyA/8CCiVAO//ao/u8gXBRjr/opJkSbGFTWHsr5MTqloGNYn9uLBT3vu8Dl9Q
jxkUHbklUlMTL1gaSopgFRBDgQZShDekaV+b3OnwUKqAhy5xM4VV/ivLgOhYYNyU
thMVTyjPxjTGe/J0S5VHzfvsS1X38nAK42c48Px/8Y01Mi1tDHlvjMxq8WsR6fPF
AkVjUejgUvLHR5eNvt/GBD7dutfQ17ZMxCvkVcvj9HQD0KGm1nwPFKMRAE/KXXqK
fJm04aTuAObYtrCpjIiMgnk72h5zd3KjMDLzG2XZJf8JLpl2USY3HIreF/zIq+Bg
MqBKOYE2kCjNpTM0nWFdpT84HcLmIgOCb8k9H3TS8kUoaipJb1qE3ct5gST+48A0
4zDppr4+1XtVWYIDSXR39Lz/nBoqDZvv3Tahtua9Nev0cNgw/wC88qebJxU0Ml59
DMDVat7vdYCGQwvBx4+sglc1+mgVy10O25BOgGTN8WQdRIA3Iecjt/SGwLv27AC4
cJZVNLi4XHxQpTWlDvHFcpP8nB0KMdEGoVT8LssniLU/oMNRv51/TCDa9nxPvTN0
8gJBvqDYvKteDtjf9gLvzl2/E35SCLfx5VhngS0ou9h/KUtNq3TzOlGzDrM6NTWQ
MHdYskcIl9DXodA7nk84vO9VFWxlYkVzj/CXlf4SJegDUAuAzbo=
=a34y
-----END PGP SIGNATURE-----
--=-=-=--