[RFC PATCH 0/2] 9p: retrieve fid from file if it exsits

[RFC PATCH 0/2] 9p: retrieve fid from file if it exsits

[Jianyong Wu]

in the current implementation in setattr in 9p, fid will always be retrieved
from dentry, which may be against the original intension of user and lead to
bug.
also, remove no used code in 9p.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>

Jianyong Wu (2):
  9p: retrive fid from file when file instance exist.
  9p: remove unused code in 9p

 fs/9p/vfs_inode.c      | 58 +++---------------------------------------
 fs/9p/vfs_inode_dotl.c |  5 +++-
 2 files changed, 8 insertions(+), 55 deletions(-)

-- 
2.17.1

[RFC PATCH 1/2] 9p: retrieve fid from file when file instance exist.

[Jianyong Wu]

In the current setattr implementation in 9p, fid will always retrieved from
dentry no matter file instance exist or not when setattr. There will
be some info related to open file instance dropped. so it's better
to retrieve fid from file instance if file instance is passed to setattr.

for example:
fd=open("tmp", O_RDWR);
ftruncate(fd, 10);

the file context related with fd info will lost as fid will always be
retrieved from dentry, then the backend can't get the info of file context.
it is against the original intention of user and may lead to bug.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 fs/9p/vfs_inode.c      | 5 ++++-
 fs/9p/vfs_inode_dotl.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index c9255d399917..010869389523 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1100,7 +1100,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
 
 	retval = -EPERM;
 	v9ses = v9fs_dentry2v9ses(dentry);
-	fid = v9fs_fid_lookup(dentry);
+	if (iattr->ia_valid & ATTR_FILE)
+		fid = iattr->ia_file->private_data;
+	else
+		fid = v9fs_fid_lookup(dentry);
 	if(IS_ERR(fid))
 		return PTR_ERR(fid);
 
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 60328b21c5fb..28fb04cbeb1a 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -560,7 +560,10 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr)
 	p9attr.mtime_sec = iattr->ia_mtime.tv_sec;
 	p9attr.mtime_nsec = iattr->ia_mtime.tv_nsec;
 
-	fid = v9fs_fid_lookup(dentry);
+	if (iattr->ia_valid & ATTR_FILE)
+		fid = iattr->ia_file->private_data;
+	else
+		fid = v9fs_fid_lookup(dentry);
 	if (IS_ERR(fid))
 		return PTR_ERR(fid);
 
-- 
2.17.1

Re: [RFC PATCH 1/2] 9p: retrieve fid from file when file instance exist.

[Dominique Martinet]

Jianyong Wu wrote on Sun, Jun 28, 2020:
> In the current setattr implementation in 9p, fid will always retrieved from
> dentry no matter file instance exist or not when setattr. There will
> be some info related to open file instance dropped. so it's better
> to retrieve fid from file instance if file instance is passed to setattr.
> 
> for example:
> fd=open("tmp", O_RDWR);
> ftruncate(fd, 10);
> 
> the file context related with fd info will lost as fid will always be
> retrieved from dentry, then the backend can't get the info of file context.
> it is against the original intention of user and may lead to bug.

I agree on principle, this makes more sense to use the file's fid.

Just a comment below, but while I'm up in commit message I'll also be
annoying with it -- please try to fix grammar mistakes for next
patches/version (mostly missing some 'be' for future passive form; but I
don't understand why you use future at all and some passive forms could
probably be made active to simplify... Anyway we're not here to discuss
English grammar but words missing out is sloppy and that gives a bad
impression for no good reason)

> 
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  fs/9p/vfs_inode.c      | 5 ++++-
>  fs/9p/vfs_inode_dotl.c | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index c9255d399917..010869389523 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -1100,7 +1100,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
>  
>  	retval = -EPERM;
>  	v9ses = v9fs_dentry2v9ses(dentry);
> -	fid = v9fs_fid_lookup(dentry);
> +	if (iattr->ia_valid & ATTR_FILE)
> +		fid = iattr->ia_file->private_data;

hmm, normally setattr cannot happen on a file that hasn't been opened so
private_data should always be set, but it doesn't cost much to play safe
and check? e.g. something like this is more conservative:

struct p9_fid *fid = NULL;
...
if (iattr->ia_valid & ATTR_FILE) {
	fid = iattr->ia_file->private_data;
	WARN_ON(!fid);
}
if (!fid)
	fid = v9fs_fid_lookup(dentry);



What do you think?

-- 
Dominique

RE: [RFC PATCH 1/2] 9p: retrieve fid from file when file instance exist.

[Jianyong Wu]

Hi Dominique,

> -----Original Message-----
> From: Dominique Martinet <asmadeus@codewreck.org>
> Sent: Sunday, June 28, 2020 2:28 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: ericvh@gmail.com; lucho@ionkov.net; v9fs-
> developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; Steve
> Capper <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
> <Justin.He@arm.com>; Wei Chen <Wei.Chen@arm.com>
> Subject: Re: [RFC PATCH 1/2] 9p: retrieve fid from file when file instance
> exist.
>
> Jianyong Wu wrote on Sun, Jun 28, 2020:
> > In the current setattr implementation in 9p, fid will always retrieved
> > from dentry no matter file instance exist or not when setattr. There
> > will be some info related to open file instance dropped. so it's
> > better to retrieve fid from file instance if file instance is passed to setattr.
> >
> > for example:
> > fd=open("tmp", O_RDWR);
> > ftruncate(fd, 10);
> >
> > the file context related with fd info will lost as fid will always be
> > retrieved from dentry, then the backend can't get the info of file context.
> > it is against the original intention of user and may lead to bug.
>
> I agree on principle, this makes more sense to use the file's fid.
>
Thanks!

> Just a comment below, but while I'm up in commit message I'll also be
> annoying with it -- please try to fix grammar mistakes for next
> patches/version (mostly missing some 'be' for future passive form; but I don't
> understand why you use future at all and some passive forms could probably
> be made active to simplify... Anyway we're not here to discuss English
> grammar but words missing out is sloppy and that gives a bad impression for
> no good reason)
>
Sorry to my poor English and thanks to point out the grammar mistakes,  I'll fix it.

> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> >  fs/9p/vfs_inode.c      | 5 ++++-
> >  fs/9p/vfs_inode_dotl.c | 5 ++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index
> > c9255d399917..010869389523 100644
> > --- a/fs/9p/vfs_inode.c
> > +++ b/fs/9p/vfs_inode.c
> > @@ -1100,7 +1100,10 @@ static int v9fs_vfs_setattr(struct dentry
> > *dentry, struct iattr *iattr)
> >
> >  retval = -EPERM;
> >  v9ses = v9fs_dentry2v9ses(dentry);
> > -fid = v9fs_fid_lookup(dentry);
> > +if (iattr->ia_valid & ATTR_FILE)
> > +fid = iattr->ia_file->private_data;
>
> hmm, normally setattr cannot happen on a file that hasn't been opened so
> private_data should always be set, but it doesn't cost much to play safe and
> check? e.g. something like this is more conservative:
>
> struct p9_fid *fid = NULL;
> ...
> if (iattr->ia_valid & ATTR_FILE) {
> fid = iattr->ia_file->private_data;
> WARN_ON(!fid);
> }
> if (!fid)
> fid = v9fs_fid_lookup(dentry);
>
>
>
> What do you think?
>
Thanks, I think it's better. I'll take it.

Thanks
Jianyong

> --
> Dominique
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[RFC PATCH 2/2] 9p: remove unused code in 9p

[Jianyong Wu]

These code has been commented out since 2007 and lied in kernel
since then. it's time to remove thest no used code.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 fs/9p/vfs_inode.c | 53 -----------------------------------------------
 1 file changed, 53 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 010869389523..23aed9047efe 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -368,59 +368,6 @@ struct inode *v9fs_get_inode(struct super_block *sb, umode_t mode, dev_t rdev)
 	return inode;
 }
 
-/*
-static struct v9fs_fid*
-v9fs_clone_walk(struct v9fs_session_info *v9ses, u32 fid, struct dentry *dentry)
-{
-	int err;
-	int nfid;
-	struct v9fs_fid *ret;
-	struct v9fs_fcall *fcall;
-
-	nfid = v9fs_get_idpool(&v9ses->fidpool);
-	if (nfid < 0) {
-		eprintk(KERN_WARNING, "no free fids available\n");
-		return ERR_PTR(-ENOSPC);
-	}
-
-	err = v9fs_t_walk(v9ses, fid, nfid, (char *) dentry->d_name.name,
-		&fcall);
-
-	if (err < 0) {
-		if (fcall && fcall->id == RWALK)
-			goto clunk_fid;
-
-		PRINT_FCALL_ERROR("walk error", fcall);
-		v9fs_put_idpool(nfid, &v9ses->fidpool);
-		goto error;
-	}
-
-	kfree(fcall);
-	fcall = NULL;
-	ret = v9fs_fid_create(v9ses, nfid);
-	if (!ret) {
-		err = -ENOMEM;
-		goto clunk_fid;
-	}
-
-	err = v9fs_fid_insert(ret, dentry);
-	if (err < 0) {
-		v9fs_fid_destroy(ret);
-		goto clunk_fid;
-	}
-
-	return ret;
-
-clunk_fid:
-	v9fs_t_clunk(v9ses, nfid);
-
-error:
-	kfree(fcall);
-	return ERR_PTR(err);
-}
-*/
-
-
 /**
  * v9fs_clear_inode - release an inode
  * @inode: inode to release
-- 
2.17.1

Re: [RFC PATCH 2/2] 9p: remove unused code in 9p

[Dominique Martinet]

Jianyong Wu wrote on Sun, Jun 28, 2020:
> These code has been commented out since 2007 and lied in kernel
> since then. it's time to remove thest no used code.

Good point, happy to take this - please resend without RFC separately
(the two commits aren't related) after fixing the commit message
(lie/lay is complicated and I'm not sure what should be used there but
lied definitely isn't correct, the qualification doesn't really matter
though so probably simpler to remove; and 'thest no used code' does not
parse)

-- 
Dominique

RE: [RFC PATCH 2/2] 9p: remove unused code in 9p

[Jianyong Wu]

Hi Dominique,

> -----Original Message-----
> From: Dominique Martinet <asmadeus@codewreck.org>
> Sent: Sunday, June 28, 2020 1:52 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: ericvh@gmail.com; lucho@ionkov.net; v9fs-
> developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; Steve
> Capper <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
> <Justin.He@arm.com>; Wei Chen <Wei.Chen@arm.com>
> Subject: Re: [RFC PATCH 2/2] 9p: remove unused code in 9p
>
> Jianyong Wu wrote on Sun, Jun 28, 2020:
> > These code has been commented out since 2007 and lied in kernel since
> > then. it's time to remove thest no used code.
>
> Good point, happy to take this - please resend without RFC separately (the
> two commits aren't related) after fixing the commit message (lie/lay is
> complicated and I'm not sure what should be used there but lied definitely
> isn't correct, the qualification doesn't really matter though so probably
> simpler to remove; and 'thest no used code' does not
> parse)
>
Thanks, I think "lay" is right. I will fix it and resend.

Thanks
Jianyong

> --
> Dominique
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.