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:     Ira Weiny <ira.weiny@intel.com>
Cc:     Evgeniy Dushistov <dushistov@mail.ru>,
        Al Viro <viro@zeniv.linux.org.uk>,
        linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v4 2/3] fs/ufs: Change the signature of ufs_get_page()
Date:   Thu, 22 Dec 2022 15:43:54 +0100
Message-ID: <2786049.Y6S9NjorxK@suse>
In-Reply-To: <Y6Pnh98KZj0D+FUR@iweiny-desk3>
References: <20221221172802.18743-1-fmdefrancesco@gmail.com>
 <20221221172802.18743-3-fmdefrancesco@gmail.com>
 <Y6Pnh98KZj0D+FUR@iweiny-desk3>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

On gioved=EC 22 dicembre 2022 06:13:43 CET Ira Weiny wrote:
> On Wed, Dec 21, 2022 at 06:28:01PM +0100, Fabio M. De Francesco wrote:
> > Change the signature of ufs_get_page() in order to prepare this function
> > to the conversion to the use of kmap_local_page(). Change also those ca=
ll
> > sites which are required to conform its invocations to the new
> > signature.
> >=20
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >=20
> >  fs/ufs/dir.c | 49 +++++++++++++++++++++----------------------------
> >  1 file changed, 21 insertions(+), 28 deletions(-)
> >=20
> > diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
> > index 69f78583c9c1..9fa86614d2d1 100644
> > --- a/fs/ufs/dir.c
> > +++ b/fs/ufs/dir.c
> > @@ -185,7 +185,7 @@ static bool ufs_check_page(struct page *page)
> >=20
> >  	return false;
> > =20
> >  }
> >=20
> > -static struct page *ufs_get_page(struct inode *dir, unsigned long n)
> > +static void *ufs_get_page(struct inode *dir, unsigned long n, struct p=
age
> > **p)>=20
> >  {
> > =20
> >  	struct address_space *mapping =3D dir->i_mapping;
> >  	struct page *page =3D read_mapping_page(mapping, n, NULL);
> >=20
> > @@ -195,8 +195,10 @@ static struct page *ufs_get_page(struct inode *dir,
> > unsigned long n)>=20
> >  			if (!ufs_check_page(page))
> >  		=09
> >  				goto fail;
> >  	=09
> >  		}
> >=20
> > +		*p =3D page;
> > +		return page_address(page);
> >=20
> >  	}
> >=20
> > -	return page;
> > +	return ERR_CAST(page);
> >=20
> >  fail:
> >  	ufs_put_page(page);
> >=20
> > @@ -227,15 +229,12 @@ ufs_next_entry(struct super_block *sb, struct
> > ufs_dir_entry *p)>=20
> >  struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p)
> >  {
> >=20
> > -	struct page *page =3D ufs_get_page(dir, 0);
> > -	struct ufs_dir_entry *de =3D NULL;
> > +	struct ufs_dir_entry *de =3D ufs_get_page(dir, 0, p);
>=20
> I don't know why but ufs_get_page() returning an address read really odd =
to
> me. But rolling around my head alternative names nothing seems better than
> this.

ufs_get_kaddr()?

> > -	if (!IS_ERR(page)) {
> > -		de =3D ufs_next_entry(dir->i_sb,
> > -				    (struct ufs_dir_entry=20
*)page_address(page));
> > -		*p =3D page;
> > -	}
> > -	return de;
> > +	if (!IS_ERR(de))
> > +		return ufs_next_entry(dir->i_sb, de);
> > +	else
> > +		return NULL;
> >=20
> >  }
> > =20
> >  /*
> >=20
> > @@ -273,11 +272,10 @@ struct ufs_dir_entry *ufs_find_entry(struct inode
> > *dir, const struct qstr *qstr,>=20
> >  		start =3D 0;
> >  =09
> >  	n =3D start;
> >  	do {
> >=20
> > -		char *kaddr;
> > -		page =3D ufs_get_page(dir, n);
> > -		if (!IS_ERR(page)) {
> > -			kaddr =3D page_address(page);
> > -			de =3D (struct ufs_dir_entry *) kaddr;
> > +		char *kaddr =3D ufs_get_page(dir, n, &page);
> > +
> > +		if (!IS_ERR(kaddr)) {
> > +			de =3D (struct ufs_dir_entry *)kaddr;
> >=20
> >  			kaddr +=3D ufs_last_byte(dir, n) - reclen;
> >  			while ((char *) de <=3D kaddr) {
> >  		=09
> >  				if (ufs_match(sb, namelen, name, de))
> >=20
> > @@ -328,12 +326,10 @@ int ufs_add_link(struct dentry *dentry, struct in=
ode
> > *inode)>=20
> >  	for (n =3D 0; n <=3D npages; n++) {
> >  =09
> >  		char *dir_end;
> >=20
> > -		page =3D ufs_get_page(dir, n);
> > -		err =3D PTR_ERR(page);
> > -		if (IS_ERR(page))
> > -			goto out;
> > +		kaddr =3D ufs_get_page(dir, n, &page);
> > +		if (IS_ERR(kaddr))
> > +			return PTR_ERR(kaddr);
> >=20
> >  		lock_page(page);
> >=20
> > -		kaddr =3D page_address(page);
> >=20
> >  		dir_end =3D kaddr + ufs_last_byte(dir, n);
> >  		de =3D (struct ufs_dir_entry *)kaddr;
> >  		kaddr +=3D PAGE_SIZE - reclen;
> >=20
> > @@ -395,7 +391,6 @@ int ufs_add_link(struct dentry *dentry, struct inode
> > *inode)>=20
> >  	/* OFFSET_CACHE */
> > =20
> >  out_put:
> >  	ufs_put_page(page);
> >=20
> > -out:
> >  	return err;
> > =20
> >  out_unlock:
> >  	unlock_page(page);
> >=20
> > @@ -429,6 +424,7 @@ ufs_readdir(struct file *file, struct dir_context=20
*ctx)
> >=20
> >  	unsigned chunk_mask =3D ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1);
> >  	bool need_revalidate =3D !inode_eq_iversion(inode, file->f_version);
> >  	unsigned flags =3D UFS_SB(sb)->s_flags;
> >=20
> > +	struct page *page;
>=20
> NIT: Does page now leave the scope of the for loop?
>=20

Strange...
I can't say why I did so.

> >  	UFSD("BEGIN\n");
> >=20
> > @@ -439,16 +435,14 @@ ufs_readdir(struct file *file, struct dir_context
> > *ctx)
> >=20
> >  		char *kaddr, *limit;
> >  		struct ufs_dir_entry *de;
>=20
> Couldn't that be declared here?

Yes, it could :-)

> Regardless I don't think this is broken.

Since I have to submit a new version of this series, there's no problem mov=
ing=20
the declaration of "page" back into the loop.

> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Thanks,

=46abio
>
> > -		struct page *page =3D ufs_get_page(inode, n);
> > -
> > -		if (IS_ERR(page)) {
> > +		kaddr =3D ufs_get_page(inode, n, &page);
> > +		if (IS_ERR(kaddr)) {
> >=20
> >  			ufs_error(sb, __func__,
> >  		=09
> >  				  "bad page in #%lu",
> >  				  inode->i_ino);
> >  		=09
> >  			ctx->pos +=3D PAGE_SIZE - offset;
> >  			return -EIO;
> >  	=09
> >  		}
> >=20
> > -		kaddr =3D page_address(page);
> >=20
> >  		if (unlikely(need_revalidate)) {
> >  	=09
> >  			if (offset) {
> >  		=09
> >  				offset =3D ufs_validate_entry(sb, kaddr,=20
offset, chunk_mask);
> >=20
> > @@ -595,12 +589,11 @@ int ufs_empty_dir(struct inode * inode)
> >=20
> >  	for (i =3D 0; i < npages; i++) {
> >  =09
> >  		char *kaddr;
> >  		struct ufs_dir_entry *de;
> >=20
> > -		page =3D ufs_get_page(inode, i);
> >=20
> > -		if (IS_ERR(page))
> > +		kaddr =3D ufs_get_page(inode, i, &page);
> > +		if (IS_ERR(kaddr))
> >=20
> >  			continue;
> >=20
> > -		kaddr =3D page_address(page);
> >=20
> >  		de =3D (struct ufs_dir_entry *)kaddr;
> >  		kaddr +=3D ufs_last_byte(inode, i) - UFS_DIR_REC_LEN(1);
> >=20
> > --
> > 2.39.0