From: NeilBrown <neilb@suse.com>
To: Jeff Layton <jlayton@redhat.com>, Matthew Wilcox <willy@infradead.org>
Date: Thu, 13 Apr 2017 07:36:17 +1000
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, tytso@mit.edu, jack@suse.cz,
        viro@zeniv.linux.org.uk, Dave Kleikamp <dave.kleikamp@oracle.com>
Subject: Re: [PATCH v2 06/17] mm: doc comment for scary spot in write_one_page
In-Reply-To: <1492012373.2937.10.camel@redhat.com>
References: <20170412120614.6111-1-jlayton@redhat.com> <20170412120614.6111-7-jlayton@redhat.com> <1492002094.2937.4.camel@redhat.com> <20170412143857.GD784@bombadil.infradead.org> <1492012373.2937.10.camel@redhat.com>
Message-ID: <87lgr5ux4e.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: 3982
Lines: 101

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

On Wed, Apr 12 2017, Jeff Layton wrote:

> On Wed, 2017-04-12 at 07:38 -0700, Matthew Wilcox wrote:
>> On Wed, Apr 12, 2017 at 09:01:34AM -0400, Jeff Layton wrote:
>> > On Wed, 2017-04-12 at 08:06 -0400, Jeff Layton wrote:
>> > > Not sure what to do here just yet.
>> > >=20
>> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> > > ---
>> > >  mm/page-writeback.c | 6 ++++++
>> > >  1 file changed, 6 insertions(+)
>> > >=20
>> > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>> > > index de0dbf12e2c1..3ac8399dc984 100644
>> > > --- a/mm/page-writeback.c
>> > > +++ b/mm/page-writeback.c
>> > > @@ -2388,6 +2388,12 @@ int write_one_page(struct page *page)
>> > >  		ret =3D mapping->a_ops->writepage(page, &wbc);
>> > >  		if (ret =3D=3D 0) {
>> > >  			wait_on_page_writeback(page);
>> > > +			/*
>> > > +			 * FIXME: is this racy? What guarantees that PG_error
>> > > +			 * will still be set once we get around to checking it?
>> > > +			 * What if writeback fails, but then a read is issued
>> > > +			 * before we check this, and that calls ClearPageError?
>> > > +			 */
>> > >  			if (PageError(page))
>> > >  				ret =3D -EIO;
>> > >  		}
>> >=20
>> > Ahh, we are always under the page lock here, and this is generally used
>> > for writing out directory pages anyway. I'm fine with dropping this
>> > patch unless someone else sees a problem here.
>>=20
>> ->writepage drops the page lock.  We're still holding a refcount on this
>> page, but that's not going to prevent read being called.  But maybe the
>> filesystem won't call read on a page that's marked as PageError?
>
> Hard to be sure there. I really wonder if that check is needed at all,
> the more I look at it. After all, we are calling writepage with
> WB_SYNC_ALL so we should get an error there.

WB_SYNC_ALL doesn't cause writepage to wait.  It might case it to ask
for REQ_SYNC, so the write requests gets priority in the block layer.
WB_SYNC_ALL does cause writepages (with an 's') to wait.
(At least, that is how I read the code).

>
> Is it also possible these pages could be written back before that point
> (due to memory pressure or something) and that fail?

Probably, in which case clear_page_dirty_for_io() will fail and
write_one_page() will just unlock the page.

>
> Maybe we should just have a call to filemap_check_errors on exiting
> this function?

I'm leaning in that direction.

>
> With the the wb_err_t based stuff, we could change it to sample the
> wb_err early, and then use that to see if an error has occurred since
> then. Maybe we should even allow callers to pass a wb_err_t in here, so
> we can report errors that have occurred since a known point?

That feels to me like over-engineering.  We would need to
unconditionally call writepage() for that to work.

We seem to be agreed that write errors for buffered writes are reported
per-address-space.  To get per-page errors you have to use direct IO.
Let's focus on that policy and make it work.

Thanks,
NeilBrown


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

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

iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAljundEACgkQOeye3VZi
gbkkXQ//SDD1iZIntZHow/1d4RgFLDTZNviDe8F1G6/DBmmoFKNwpzgeKSRUL8rj
/Gt0BQw8GkfchZRT/nsBu+KTqeOhS7IAOKF7YSbqWgmfr0eY4IhsiFvdzrPY6SC+
qdhGlJm0MsozlyxLpoTF27sf2gtILJTZ2lRqqfGb88O9e5CFd7nRpV46yXUzF0hm
hhrdNCxCLplCapCT2Q6HU09W/LS7EQBfXrWQ8xf0EOaS7KKgS5R5oIAjLKcszzGD
O5EfNGDagV9xz+iw7Po87IoFnueMUWcrMsKIoeUPhHAgXOqkngiJr/uwIvhBvk/Q
CkunjA6MbM4yDjqcFqSVHaLtzOOfGsEo+N/90BCQkPcBexTqsu40sMH2E3KjcLY1
WLJGnhdxKz/juZAnz9FXK4lHMG4NtQk0qCIouiZtFLcZ4VRGuMIb+6FF6V3jCqQs
7ITmLfh8MORWtB3MbBEQ3efvnZnMbmPrZZBQj+fTIh+myYFKDYA2hr2//nfolNx7
9SnMVnFtxMAI0LXa/0gHCJJthK3AP+HiRbt4Kwu/ZE4ZtNQZRs1F451kMWH24yhf
YxBIKPHjlauI9lYD0+EmzQaUZOczosvW9mQzlA9M+q1IUempyVgO3dRa/R8jDg7L
y/Tn+eAK4qC5w58yMjRYgFaVDMqa699Jl1dOlOk06AcpNBwpZIs=
=AoXM
-----END PGP SIGNATURE-----
--=-=-=--