| Message ID | 20230524100812.80741-1-bo.wu@vivo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [f2fs-dev,1/1] f2fs: fix args passed to trace_f2fs_lookup_end | expand |
On 05/24, Wu Bo wrote: > The NULL return of 'd_splice_alias' dosen't mean error. > > Signed-off-by: Wu Bo <bo.wu@vivo.com> > --- > fs/f2fs/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > index 77a71276ecb1..e5a3e39ce90c 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -577,7 +577,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, > #endif > new = d_splice_alias(inode, dentry); > err = PTR_ERR_OR_ZERO(new); > - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); > + trace_f2fs_lookup_end(dir, new ? new : dentry, ino, err); Shouldn't give an error pointer to the dentry field. How about just giving the err? - err = PTR_ERR_OR_ZERO(new); - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); + trace_f2fs_lookup_end(dir, dentry, ino, PTR_ERR_OR_ZERO(new)); > return new; > out_iput: > iput(inode); > -- > 2.35.3
On 2023/5/27 1:21, Jaegeuk Kim wrote: > On 05/24, Wu Bo wrote: >> The NULL return of 'd_splice_alias' dosen't mean error. >> >> Signed-off-by: Wu Bo <bo.wu@vivo.com> >> --- >> fs/f2fs/namei.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c >> index 77a71276ecb1..e5a3e39ce90c 100644 >> --- a/fs/f2fs/namei.c >> +++ b/fs/f2fs/namei.c >> @@ -577,7 +577,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, >> #endif >> new = c(inode, dentry); >> err = PTR_ERR_OR_ZERO(new); >> - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); >> + trace_f2fs_lookup_end(dir, new ? new : dentry, ino, err); > > Shouldn't give an error pointer to the dentry field. > > How about just giving the err? > > - err = PTR_ERR_OR_ZERO(new); > - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); > + trace_f2fs_lookup_end(dir, dentry, ino, PTR_ERR_OR_ZERO(new)); static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr) { if (IS_ERR(ptr)) return PTR_ERR(ptr); else return 0; } For below two cases, PTR_ERR_OR_ZERO(new) will return zero: a) f2fs_lookup found existed dentry b) f2fs_lookup didn't find existed dentry (-ENOENT case) So in below commit, I passed -ENOENT to tracepoint for case b), so we can distinguish result of f2fs_lookup in tracepoint, actually, -ENOENT is expected value when we create a new file/directory. Commit 84597b1f9b05 ("f2fs: fix wrong value of tracepoint parameter") > > >> return new; >> out_iput: >> iput(inode); >> -- >> 2.35.3
On Sat, May 27, 2023 at 09:01:41AM +0800, Chao Yu wrote: > On 2023/5/27 1:21, Jaegeuk Kim wrote: > > On 05/24, Wu Bo wrote: > > > The NULL return of 'd_splice_alias' dosen't mean error. > > > > > > Signed-off-by: Wu Bo <bo.wu@vivo.com> > > > --- > > > fs/f2fs/namei.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > > > index 77a71276ecb1..e5a3e39ce90c 100644 > > > --- a/fs/f2fs/namei.c > > > +++ b/fs/f2fs/namei.c > > > @@ -577,7 +577,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, > > > #endif > > > new = c(inode, dentry); > > > err = PTR_ERR_OR_ZERO(new); > > > - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); > > > + trace_f2fs_lookup_end(dir, new ? new : dentry, ino, err); > > > > Shouldn't give an error pointer to the dentry field. > > > > How about just giving the err? > > > > - err = PTR_ERR_OR_ZERO(new); > > - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); > > + trace_f2fs_lookup_end(dir, dentry, ino, PTR_ERR_OR_ZERO(new)); > > static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr) > { > if (IS_ERR(ptr)) > return PTR_ERR(ptr); > else > return 0; > } > > For below two cases, PTR_ERR_OR_ZERO(new) will return zero: > a) f2fs_lookup found existed dentry > b) f2fs_lookup didn't find existed dentry (-ENOENT case) > > So in below commit, I passed -ENOENT to tracepoint for case b), so we can > distinguish result of f2fs_lookup in tracepoint, actually, -ENOENT is expected > value when we create a new file/directory. > > Commit 84597b1f9b05 ("f2fs: fix wrong value of tracepoint parameter") I can see this commit is try to distinguish the dentry not existed case. But a normal case which dentry is exactly found will also go through 'd_splice_alias', and its return is also NULL. This makes the tracepoint always print 'err:-2' like the following: ls-11676 [004] .... 329281.943118: f2fs_lookup_end: dev = (254,39), pino = 4451, name:Alarms, ino:7093, err:-2 ls-11676 [004] .... 329281.943145: f2fs_lookup_end: dev = (254,39), pino = 4451, name:Notifications, ino:7094, err:-2 ls-11676 [004] .... 329281.943172: f2fs_lookup_end: dev = (254,39), pino = 4451, name:Pictures, ino:7095, err:-2 Even these lookup are acctually successful, this is a bit strange. > > > > > > > > return new; > > > out_iput: > > > iput(inode); > > > -- > > > 2.35.3 > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2023/5/29 12:13, Wu Bo wrote: > On Sat, May 27, 2023 at 09:01:41AM +0800, Chao Yu wrote: >> On 2023/5/27 1:21, Jaegeuk Kim wrote: >>> On 05/24, Wu Bo wrote: >>>> The NULL return of 'd_splice_alias' dosen't mean error. >>>> >>>> Signed-off-by: Wu Bo <bo.wu@vivo.com> >>>> --- >>>> fs/f2fs/namei.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c >>>> index 77a71276ecb1..e5a3e39ce90c 100644 >>>> --- a/fs/f2fs/namei.c >>>> +++ b/fs/f2fs/namei.c >>>> @@ -577,7 +577,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, >>>> #endif >>>> new = c(inode, dentry); >>>> err = PTR_ERR_OR_ZERO(new); >>>> - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); >>>> + trace_f2fs_lookup_end(dir, new ? new : dentry, ino, err); >>> >>> Shouldn't give an error pointer to the dentry field. >>> >>> How about just giving the err? >>> >>> - err = PTR_ERR_OR_ZERO(new); >>> - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); >>> + trace_f2fs_lookup_end(dir, dentry, ino, PTR_ERR_OR_ZERO(new)); >> >> static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr) >> { >> if (IS_ERR(ptr)) >> return PTR_ERR(ptr); >> else >> return 0; >> } >> >> For below two cases, PTR_ERR_OR_ZERO(new) will return zero: >> a) f2fs_lookup found existed dentry >> b) f2fs_lookup didn't find existed dentry (-ENOENT case) >> >> So in below commit, I passed -ENOENT to tracepoint for case b), so we can >> distinguish result of f2fs_lookup in tracepoint, actually, -ENOENT is expected >> value when we create a new file/directory. >> >> Commit 84597b1f9b05 ("f2fs: fix wrong value of tracepoint parameter") > I can see this commit is try to distinguish the dentry not existed case. > But a normal case which dentry is exactly found will also go through > 'd_splice_alias', and its return is also NULL. This makes the tracepoint always > print 'err:-2' like the following: > ls-11676 [004] .... 329281.943118: f2fs_lookup_end: dev = (254,39), pino = 4451, name:Alarms, ino:7093, err:-2 > ls-11676 [004] .... 329281.943145: f2fs_lookup_end: dev = (254,39), pino = 4451, name:Notifications, ino:7094, err:-2 > ls-11676 [004] .... 329281.943172: f2fs_lookup_end: dev = (254,39), pino = 4451, name:Pictures, ino:7095, err:-2 > Even these lookup are acctually successful, this is a bit strange. Ah, I misunderstand return value's meaning of .lookup. So, how about this? it only update err if d_splice_alias() returns a negative value? if (IS_ERR(new)) err = PTR_ERR(new); trace_f2fs_lookup_end(dir, new ? new : dentry, ino, err); Thanks, >> >>> >>> >>>> return new; >>>> out_iput: >>>> iput(inode); >>>> -- >>>> 2.35.3 >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2023/5/29 18:18, Chao Yu wrote: > On 2023/5/29 12:13, Wu Bo wrote: >> On Sat, May 27, 2023 at 09:01:41AM +0800, Chao Yu wrote: >>> On 2023/5/27 1:21, Jaegeuk Kim wrote: >>>> On 05/24, Wu Bo wrote: >>>>> The NULL return of 'd_splice_alias' dosen't mean error. >>>>> >>>>> Signed-off-by: Wu Bo <bo.wu@vivo.com> >>>>> --- >>>>> fs/f2fs/namei.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c >>>>> index 77a71276ecb1..e5a3e39ce90c 100644 >>>>> --- a/fs/f2fs/namei.c >>>>> +++ b/fs/f2fs/namei.c >>>>> @@ -577,7 +577,7 @@ static struct dentry *f2fs_lookup(struct inode >>>>> *dir, struct dentry *dentry, >>>>> #endif >>>>> new = c(inode, dentry); >>>>> err = PTR_ERR_OR_ZERO(new); >>>>> - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); >>>>> + trace_f2fs_lookup_end(dir, new ? new : dentry, ino, err); >>>> >>>> Shouldn't give an error pointer to the dentry field. >>>> >>>> How about just giving the err? >>>> >>>> - err = PTR_ERR_OR_ZERO(new); >>>> - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); >>>> + trace_f2fs_lookup_end(dir, dentry, ino, PTR_ERR_OR_ZERO(new)); >>> >>> static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr) >>> { >>> if (IS_ERR(ptr)) >>> return PTR_ERR(ptr); >>> else >>> return 0; >>> } >>> >>> For below two cases, PTR_ERR_OR_ZERO(new) will return zero: >>> a) f2fs_lookup found existed dentry >>> b) f2fs_lookup didn't find existed dentry (-ENOENT case) >>> >>> So in below commit, I passed -ENOENT to tracepoint for case b), so >>> we can >>> distinguish result of f2fs_lookup in tracepoint, actually, -ENOENT >>> is expected >>> value when we create a new file/directory. >>> >>> Commit 84597b1f9b05 ("f2fs: fix wrong value of tracepoint parameter") >> I can see this commit is try to distinguish the dentry not existed case. >> But a normal case which dentry is exactly found will also go through >> 'd_splice_alias', and its return is also NULL. This makes the >> tracepoint always >> print 'err:-2' like the following: >> ls-11676 [004] .... 329281.943118: f2fs_lookup_end: dev = >> (254,39), pino = 4451, name:Alarms, ino:7093, err:-2 >> ls-11676 [004] .... 329281.943145: f2fs_lookup_end: dev = >> (254,39), pino = 4451, name:Notifications, ino:7094, err:-2 >> ls-11676 [004] .... 329281.943172: f2fs_lookup_end: dev = >> (254,39), pino = 4451, name:Pictures, ino:7095, err:-2 >> Even these lookup are acctually successful, this is a bit strange. > > Ah, I misunderstand return value's meaning of .lookup. > > So, how about this? it only update err if d_splice_alias() returns a > negative > value? > > if (IS_ERR(new)) > err = PTR_ERR(new); > trace_f2fs_lookup_end(dir, new ? new : dentry, ino, err); > > Thanks, > Yes, this will be better. >>> >>>> >>>> >>>>> return new; >>>>> out_iput: >>>>> iput(inode); >>>>> -- >>>>> 2.35.3 >>> >>> >>> _______________________________________________ >>> Linux-f2fs-devel mailing list >>> Linux-f2fs-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c index 77a71276ecb1..e5a3e39ce90c 100644 --- a/fs/f2fs/namei.c +++ b/fs/f2fs/namei.c @@ -577,7 +577,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry, #endif new = d_splice_alias(inode, dentry); err = PTR_ERR_OR_ZERO(new); - trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err); + trace_f2fs_lookup_end(dir, new ? new : dentry, ino, err); return new; out_iput: iput(inode);
The NULL return of 'd_splice_alias' dosen't mean error. Signed-off-by: Wu Bo <bo.wu@vivo.com> --- fs/f2fs/namei.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)