Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Al Viro <viro@zeniv.linux.org.uk>,
        Al Viro <viro@zeniv.linux.org.uk>
Cc:     Evgeniy Dushistov <dushistov@mail.ru>,
        Ira Weiny <ira.weiny@intel.com>, linux-kernel@vger.kernel.org,
        Jeff Moyer <jmoyer@redhat.com>,
        Evgeniy Dushistov <dushistov@mail.ru>,
        Ira Weiny <ira.weiny@intel.com>, linux-kernel@vger.kernel.org,
        Jeff Moyer <jmoyer@redhat.com>
Subject: Re: [PATCH v2 2/3] fs/ufs: Change the signature of ufs_get_page()
Date:   Tue, 13 Dec 2022 20:08:22 +0100
Message-ID: <3174926.0WQXIW03uk@suse>
In-Reply-To: <Y5glgpD7fFifC4Fi@ZenIV>
References: <Y5glgpD7fFifC4Fi@ZenIV>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

On marted=EC 13 dicembre 2022 08:10:58 CET Al Viro wrote:
> On Tue, Dec 13, 2022 at 12:19:05AM +0100, Fabio M. De Francesco wrote:
> > +static void *ufs_get_page(struct inode *dir, unsigned long n, struct p=
age
> > **page)>=20
> >  {
> > =20
> >  	struct address_space *mapping =3D dir->i_mapping;
> >=20
> > -	struct page *page =3D read_mapping_page(mapping, n, NULL);
> > -	if (!IS_ERR(page)) {
> > -		kmap(page);
> > -		if (unlikely(!PageChecked(page))) {
> > -			if (!ufs_check_page(page))
> > +	*page =3D read_mapping_page(mapping, n, NULL);
> > +	if (!IS_ERR(*page)) {
> > +		kmap(*page);
> > +		if (unlikely(!PageChecked(*page))) {
> > +			if (!ufs_check_page(*page))
> >=20
> >  				goto fail;
> >  	=09
> >  		}
> >  =09
> >  	}
> >=20
> > -	return page;
> > +	return page_address(*page);
>=20
> Er...   You really don't want to do that when you've got ERR_PTR()
> from read_mapping_page().
>=20

Sure :-)

Obviously, I'll fix it ASAP.

But I can't yet understand why that pointer was returned unconditionally in=
=20
the code which I'm changing with this patch (i.e., regardless of any potent=
ial =20
errors from read_mapping_page()) :-/=20

>
> >  fail:
> > -	ufs_put_page(page);
> > +	ufs_put_page(*page);
> >=20
> >  	return ERR_PTR(-EIO);
> > =20
> >  }
>=20
> IDGI...
>=20
> static void *ufs_get_page(struct inode *dir, unsigned long n, struct page=
=20
**p)
> {
> 	struct address_space *mapping =3D dir->i_mapping;
> 	struct page *page =3D read_mapping_page(mapping, n, NULL);
>=20
> 	if (!IS_ERR(page)) {
> 		kmap(page);
> 		if (unlikely(!PageChecked(page))) {
> 			if (!ufs_check_page(page))
> 				goto fail;
> 		}
> 		*p =3D page;
> 		return page_address(page);
> 	}
> 	return ERR_CAST(page);
>=20
> fail:
> 	ufs_put_page(page);
> 	return ERR_PTR(-EIO);
> }
>=20
> all there is to it...  The only things you need to change are
> 1) type of function
> 2) make sure to store the page into that pointer to pointer to page on=20
success
> 3) return page_address(page) instead of page on success
> 4) use ERR_CAST() to convert ERR_PTR() that is struct page * into equal
> ERR_PTR() that is void * (the last one is optional, just makes the intent
> more clear).

I've never seen anything like this: you are spoon feeding me :-)

Please don't misunderstand: I'm OK with it and, above all, I'm surely =20
appreciating how much patience and time you are devolving to help me.

I'll send v3 ASAP (for sure within the next 24 hours).

=46urthermore, if there are no other issues, I'd start ASAP with the=20
decomposition of the patch to fs/sysv into a series of three units and rewo=
rk=20
the whole design in accordance to the suggestions you have been kindly=20
providing.

I'm sorry for responding and reacting so slowly. Aside from being slow beca=
use=20
of a fundamental lack of knowledge and experience, I can do Kernel developm=
ent=20
(including studying new subjects and code, doing patches, doing reviews, an=
d=20
the likes) about 7 hours a week. This is all the time I can set aside until=
 I=20
quit one of my jobs at the end of January.

Again thanks,

=46abio

P.S.: While at this patch I'd like to gently ping you about another convers=
ion=20
that I sent (and resent) months ago:

[RESEND PATCH] fs/aio: Replace kmap{,_atomic}() with kmap_local_page() at:
https://lore.kernel.org/lkml/20221016150656.5803-1-fmdefrancesco@gmail.com/

This patch to fs/aio.c has already received two "Reviewed-by" tags: the fir=
st=20
from Ira Weiny and the second from Jeff Moyer (you won't see the second the=
re,=20
because I forgot to forward it when I sent again :-( ).

The tag from Jeff is in the following message (from another [RESEND PATCH]=
=20
thread):
https://lore.kernel.org/lkml/x49h6zzvn1a.fsf@segfault.boston.devel.redhat.c=
om/

In that same thread, I explained better I meant in the last sentence of the=
=20
commit message, because Jeff commented that it is ambiguous (I'm adding him=
 in=20
the Cc list of recipients).

The original patch, submitted on Thu, 7 Jul 2022 01:33:28 +0200 is at:
https://lore.kernel.org/lkml/20220706233328.18582-1-fmdefrancesco@gmail.com/

I'd appreciate very much if you can spend some time on that patch too :)