| Message ID | 20230613085250.3648491-1-heyunlei@oppo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev] f2fs: not allowed to set file both cold and hot | expand |
On 2023/6/13 16:52, Yunlei He wrote: > File set both cold and hot advise bit is confusion, so > return EINVAL to avoid this case. > > Signed-off-by: Yunlei He <heyunlei@oppo.com> > --- > fs/f2fs/xattr.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c > index 213805d3592c..917f3ac9f1a1 100644 > --- a/fs/f2fs/xattr.c > +++ b/fs/f2fs/xattr.c > @@ -127,6 +127,9 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, > return -EINVAL; > > new_advise = new_advise & FADVISE_MODIFIABLE_BITS; > + if ((new_advise & FADVISE_COLD_BIT) && (new_advise & FADVISE_HOT_BIT)) > + return -EINVAL; Yunlei, What about the below case: 1. f2fs_xattr_advise_set(FADVISE_COLD_BIT) 2. f2fs_xattr_advise_set(FADVISE_HOT_BIT) Thanks, > + > new_advise |= old_advise & ~FADVISE_MODIFIABLE_BITS; > > F2FS_I(inode)->i_advise = new_advise; > --
On 2023/6/20 8:33, Chao Yu wrote: > On 2023/6/13 16:52, Yunlei He wrote: >> File set both cold and hot advise bit is confusion, so >> return EINVAL to avoid this case. >> >> Signed-off-by: Yunlei He <heyunlei@oppo.com> >> --- >> fs/f2fs/xattr.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c >> index 213805d3592c..917f3ac9f1a1 100644 >> --- a/fs/f2fs/xattr.c >> +++ b/fs/f2fs/xattr.c >> @@ -127,6 +127,9 @@ static int f2fs_xattr_advise_set(const struct >> xattr_handler *handler, >> return -EINVAL; >> >> new_advise = new_advise & FADVISE_MODIFIABLE_BITS; >> + if ((new_advise & FADVISE_COLD_BIT) && (new_advise & >> FADVISE_HOT_BIT)) >> + return -EINVAL; > > Yunlei, > > What about the below case: > > 1. f2fs_xattr_advise_set(FADVISE_COLD_BIT) > 2. f2fs_xattr_advise_set(FADVISE_HOT_BIT) Hi, Chao, I test this case work well with this patch, case below will return -EINVAL: f2fs_xattr_advise_set(FADVISE_COLD_BIT | FADVISE_HOT_BIT) Thanks, > > Thanks, > >> + >> new_advise |= old_advise & ~FADVISE_MODIFIABLE_BITS; >> >> F2FS_I(inode)->i_advise = new_advise; >> --
On 2023/6/20 10:42, 何云蕾(Yunlei he) wrote: > > On 2023/6/20 8:33, Chao Yu wrote: >> On 2023/6/13 16:52, Yunlei He wrote: >>> File set both cold and hot advise bit is confusion, so >>> return EINVAL to avoid this case. >>> >>> Signed-off-by: Yunlei He <heyunlei@oppo.com> >>> --- >>> fs/f2fs/xattr.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c >>> index 213805d3592c..917f3ac9f1a1 100644 >>> --- a/fs/f2fs/xattr.c >>> +++ b/fs/f2fs/xattr.c >>> @@ -127,6 +127,9 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, >>> return -EINVAL; >>> >>> new_advise = new_advise & FADVISE_MODIFIABLE_BITS; >>> + if ((new_advise & FADVISE_COLD_BIT) && (new_advise & FADVISE_HOT_BIT)) >>> + return -EINVAL; >> >> Yunlei, >> >> What about the below case: >> >> 1. f2fs_xattr_advise_set(FADVISE_COLD_BIT) >> 2. f2fs_xattr_advise_set(FADVISE_HOT_BIT) > > Hi, Chao, > > I test this case work well with this patch, case below will return -EINVAL: > > f2fs_xattr_advise_set(FADVISE_COLD_BIT | FADVISE_HOT_BIT) Correct, I missed to check below statement. new_advise |= old_advise & ~FADVISE_MODIFIABLE_BITS; Anyway, the patch looks good to me. Reviewed-by: Chao Yu <chao@kernel.org> Thanks,
On 06/20, Chao Yu wrote: > On 2023/6/20 10:42, 何云蕾(Yunlei he) wrote: > > > > On 2023/6/20 8:33, Chao Yu wrote: > > > On 2023/6/13 16:52, Yunlei He wrote: > > > > File set both cold and hot advise bit is confusion, so > > > > return EINVAL to avoid this case. > > > > > > > > Signed-off-by: Yunlei He <heyunlei@oppo.com> > > > > --- > > > > fs/f2fs/xattr.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c > > > > index 213805d3592c..917f3ac9f1a1 100644 > > > > --- a/fs/f2fs/xattr.c > > > > +++ b/fs/f2fs/xattr.c > > > > @@ -127,6 +127,9 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, > > > > return -EINVAL; > > > > > > > > new_advise = new_advise & FADVISE_MODIFIABLE_BITS; > > > > + if ((new_advise & FADVISE_COLD_BIT) && (new_advise & FADVISE_HOT_BIT)) > > > > + return -EINVAL; Why not this to allow setting one bit only? @@ -123,7 +123,8 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, return -EINVAL; new_advise = *(char *)value; - if (new_advise & ~FADVISE_MODIFIABLE_BITS) + if (new_advise & ~FADVISE_MODIFIABLE_BITS || + new_advise == FADVISE_MODIFIABLE_BITS) return -EINVAL; > > > > > > Yunlei, > > > > > > What about the below case: > > > > > > 1. f2fs_xattr_advise_set(FADVISE_COLD_BIT) > > > 2. f2fs_xattr_advise_set(FADVISE_HOT_BIT) > > > > Hi, Chao, > > > > I test this case work well with this patch, case below will return -EINVAL: > > > > f2fs_xattr_advise_set(FADVISE_COLD_BIT | FADVISE_HOT_BIT) > > Correct, I missed to check below statement. > > new_advise |= old_advise & ~FADVISE_MODIFIABLE_BITS; > > Anyway, the patch looks good to me. > > Reviewed-by: Chao Yu <chao@kernel.org> > > Thanks,
On 06/25, 何云蕾(Yunlei he) wrote: > > On 2023/6/24 3:07, Jaegeuk Kim wrote: > > On 06/20, Chao Yu wrote: > > > On 2023/6/20 10:42, 何云蕾(Yunlei he) wrote: > > > > On 2023/6/20 8:33, Chao Yu wrote: > > > > > On 2023/6/13 16:52, Yunlei He wrote: > > > > > > File set both cold and hot advise bit is confusion, so > > > > > > return EINVAL to avoid this case. > > > > > > > > > > > > Signed-off-by: Yunlei He<heyunlei@oppo.com> > > > > > > --- > > > > > > fs/f2fs/xattr.c | 3 +++ > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c > > > > > > index 213805d3592c..917f3ac9f1a1 100644 > > > > > > --- a/fs/f2fs/xattr.c > > > > > > +++ b/fs/f2fs/xattr.c > > > > > > @@ -127,6 +127,9 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, > > > > > > return -EINVAL; > > > > > > > > > > > > new_advise = new_advise & FADVISE_MODIFIABLE_BITS; > > > > > > + if ((new_advise & FADVISE_COLD_BIT) && (new_advise & FADVISE_HOT_BIT)) > > > > > > + return -EINVAL; > > Why not this to allow setting one bit only? > > > > @@ -123,7 +123,8 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, > > return -EINVAL; > > > > new_advise = *(char *)value; > > - if (new_advise & ~FADVISE_MODIFIABLE_BITS) > > + if (new_advise & ~FADVISE_MODIFIABLE_BITS || > > + new_advise == FADVISE_MODIFIABLE_BITS) > > return -EINVAL; > > Hi,Jaegeuk, > > If new modifiable advise bit is added in the future, maybe multi-bits > is allowed? > Looks like making a single bit assumption would be better in general at this moment. > Thanks > > > > > > > > Yunlei, > > > > > > > > > > What about the below case: > > > > > > > > > > 1. f2fs_xattr_advise_set(FADVISE_COLD_BIT) > > > > > 2. f2fs_xattr_advise_set(FADVISE_HOT_BIT) > > > > Hi, Chao, > > > > > > > > I test this case work well with this patch, case below will return -EINVAL: > > > > > > > > f2fs_xattr_advise_set(FADVISE_COLD_BIT | FADVISE_HOT_BIT) > > > Correct, I missed to check below statement. > > > > > > new_advise |= old_advise & ~FADVISE_MODIFIABLE_BITS; > > > > > > Anyway, the patch looks good to me. > > > > > > Reviewed-by: Chao Yu<chao@kernel.org> > > > > > > Thanks,
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c index 213805d3592c..917f3ac9f1a1 100644 --- a/fs/f2fs/xattr.c +++ b/fs/f2fs/xattr.c @@ -127,6 +127,9 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, return -EINVAL; new_advise = new_advise & FADVISE_MODIFIABLE_BITS; + if ((new_advise & FADVISE_COLD_BIT) && (new_advise & FADVISE_HOT_BIT)) + return -EINVAL; + new_advise |= old_advise & ~FADVISE_MODIFIABLE_BITS; F2FS_I(inode)->i_advise = new_advise;
File set both cold and hot advise bit is confusion, so return EINVAL to avoid this case. Signed-off-by: Yunlei He <heyunlei@oppo.com> --- fs/f2fs/xattr.c | 3 +++ 1 file changed, 3 insertions(+) -- 2.40.1