* [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE
@ 2020-07-10 2:15 Daeho Jeong
2020-07-10 3:02 ` [f2fs-dev] " Jaegeuk Kim
0 siblings, 1 reply; 14+ messages in thread
From: Daeho Jeong @ 2020-07-10 2:15 UTC (permalink / raw)
To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong
From: Daeho Jeong <daehojeong@google.com>
Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE.
1. Added -1 value support for range.len to signify the end of file.
2. If the end of the range passes over the end of file, it means until
the end of file.
3. ignored the case of that range.len is zero to prevent the function
from making end_addr zero and triggering different behaviour of
the function.
Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
fs/f2fs/file.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 368c80f8e2a1..1c4601f99326 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
file_start_write(filp);
inode_lock(inode);
- if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) {
+ if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) ||
+ range.start >= inode->i_size) {
ret = -EINVAL;
goto err;
}
- if (range.start >= inode->i_size) {
- ret = -EINVAL;
+ if (range.len == 0)
goto err;
- }
- if (inode->i_size - range.start < range.len) {
- ret = -E2BIG;
- goto err;
- }
- end_addr = range.start + range.len;
+ if (range.len == (u64)-1 || inode->i_size - range.start < range.len)
+ end_addr = inode->i_size;
+ else
+ end_addr = range.start + range.len;
to_end = (end_addr == inode->i_size);
if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) ||
--
2.27.0.383.g050319c2ae-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 2:15 [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE Daeho Jeong @ 2020-07-10 3:02 ` Jaegeuk Kim 2020-07-10 3:17 ` Daeho Jeong 2020-07-10 3:21 ` Chao Yu 0 siblings, 2 replies; 14+ messages in thread From: Jaegeuk Kim @ 2020-07-10 3:02 UTC (permalink / raw) To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong On 07/10, Daeho Jeong wrote: > From: Daeho Jeong <daehojeong@google.com> > > Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. > 1. Added -1 value support for range.len to signify the end of file. > 2. If the end of the range passes over the end of file, it means until > the end of file. > 3. ignored the case of that range.len is zero to prevent the function > from making end_addr zero and triggering different behaviour of > the function. > > Signed-off-by: Daeho Jeong <daehojeong@google.com> > --- > fs/f2fs/file.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 368c80f8e2a1..1c4601f99326 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) > file_start_write(filp); > inode_lock(inode); > > - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { > + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || > + range.start >= inode->i_size) { > ret = -EINVAL; > goto err; > } > > - if (range.start >= inode->i_size) { > - ret = -EINVAL; > + if (range.len == 0) > goto err; > - } > > - if (inode->i_size - range.start < range.len) { > - ret = -E2BIG; > - goto err; > - } > - end_addr = range.start + range.len; > + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) > + end_addr = inode->i_size; Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for ending criteria? > + else > + end_addr = range.start + range.len; > > to_end = (end_addr == inode->i_size); > if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || > -- > 2.27.0.383.g050319c2ae-goog > > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:02 ` [f2fs-dev] " Jaegeuk Kim @ 2020-07-10 3:17 ` Daeho Jeong 2020-07-10 3:20 ` Jaegeuk Kim 2020-07-10 3:21 ` Chao Yu 1 sibling, 1 reply; 14+ messages in thread From: Daeho Jeong @ 2020-07-10 3:17 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong 1. The valid data will be within i_size. 2. All the trim operations will be done in a unit of block, even if i_size is not aligned with BLKSIZE like the below. index = F2FS_BYTES_TO_BLK(range.start); pg_end = DIV_ROUND_UP(end_addr, F2FS_BLKSIZE); <= BLKSIZE aligned Are you worried about the case that sudden power-off occurs while a file is being truncated? ex) 1GB file is being truncated to 4KB -> sudden power-off -> i_size(4KB), i_blocks(maybe somewhere between 4KB and 1GB) 2020년 7월 10일 (금) 오후 12:02, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성: > > On 07/10, Daeho Jeong wrote: > > From: Daeho Jeong <daehojeong@google.com> > > > > Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. > > 1. Added -1 value support for range.len to signify the end of file. > > 2. If the end of the range passes over the end of file, it means until > > the end of file. > > 3. ignored the case of that range.len is zero to prevent the function > > from making end_addr zero and triggering different behaviour of > > the function. > > > > Signed-off-by: Daeho Jeong <daehojeong@google.com> > > --- > > fs/f2fs/file.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > index 368c80f8e2a1..1c4601f99326 100644 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) > > file_start_write(filp); > > inode_lock(inode); > > > > - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { > > + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || > > + range.start >= inode->i_size) { > > ret = -EINVAL; > > goto err; > > } > > > > - if (range.start >= inode->i_size) { > > - ret = -EINVAL; > > + if (range.len == 0) > > goto err; > > - } > > > > - if (inode->i_size - range.start < range.len) { > > - ret = -E2BIG; > > - goto err; > > - } > > - end_addr = range.start + range.len; > > + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) > > + end_addr = inode->i_size; > > Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for > ending criteria? > > > + else > > + end_addr = range.start + range.len; > > > > to_end = (end_addr == inode->i_size); > > if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || > > -- > > 2.27.0.383.g050319c2ae-goog > > > > > > > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:17 ` Daeho Jeong @ 2020-07-10 3:20 ` Jaegeuk Kim 2020-07-10 3:25 ` Eric Biggers 0 siblings, 1 reply; 14+ messages in thread From: Jaegeuk Kim @ 2020-07-10 3:20 UTC (permalink / raw) To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong On 07/10, Daeho Jeong wrote: > 1. The valid data will be within i_size. > 2. All the trim operations will be done in a unit of block, even if > i_size is not aligned with BLKSIZE like the below. > > index = F2FS_BYTES_TO_BLK(range.start); > pg_end = DIV_ROUND_UP(end_addr, F2FS_BLKSIZE); <= BLKSIZE aligned > > Are you worried about the case that sudden power-off occurs while a > file is being truncated? > ex) 1GB file is being truncated to 4KB -> sudden power-off -> > i_size(4KB), i_blocks(maybe somewhere between 4KB and 1GB) Yes. Basically, I believe we can have some data beyond i_size like fsverity. > > 2020년 7월 10일 (금) 오후 12:02, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성: > > > > On 07/10, Daeho Jeong wrote: > > > From: Daeho Jeong <daehojeong@google.com> > > > > > > Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. > > > 1. Added -1 value support for range.len to signify the end of file. > > > 2. If the end of the range passes over the end of file, it means until > > > the end of file. > > > 3. ignored the case of that range.len is zero to prevent the function > > > from making end_addr zero and triggering different behaviour of > > > the function. > > > > > > Signed-off-by: Daeho Jeong <daehojeong@google.com> > > > --- > > > fs/f2fs/file.c | 16 +++++++--------- > > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > index 368c80f8e2a1..1c4601f99326 100644 > > > --- a/fs/f2fs/file.c > > > +++ b/fs/f2fs/file.c > > > @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) > > > file_start_write(filp); > > > inode_lock(inode); > > > > > > - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { > > > + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || > > > + range.start >= inode->i_size) { > > > ret = -EINVAL; > > > goto err; > > > } > > > > > > - if (range.start >= inode->i_size) { > > > - ret = -EINVAL; > > > + if (range.len == 0) > > > goto err; > > > - } > > > > > > - if (inode->i_size - range.start < range.len) { > > > - ret = -E2BIG; > > > - goto err; > > > - } > > > - end_addr = range.start + range.len; > > > + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) > > > + end_addr = inode->i_size; > > > > Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for > > ending criteria? > > > > > + else > > > + end_addr = range.start + range.len; > > > > > > to_end = (end_addr == inode->i_size); > > > if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || > > > -- > > > 2.27.0.383.g050319c2ae-goog > > > > > > > > > > > > _______________________________________________ > > > Linux-f2fs-devel mailing list > > > Linux-f2fs-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:20 ` Jaegeuk Kim @ 2020-07-10 3:25 ` Eric Biggers 2020-07-10 3:29 ` Jaegeuk Kim 0 siblings, 1 reply; 14+ messages in thread From: Eric Biggers @ 2020-07-10 3:25 UTC (permalink / raw) To: Jaegeuk Kim Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On Thu, Jul 09, 2020 at 08:20:35PM -0700, Jaegeuk Kim wrote: > On 07/10, Daeho Jeong wrote: > > 1. The valid data will be within i_size. > > 2. All the trim operations will be done in a unit of block, even if > > i_size is not aligned with BLKSIZE like the below. > > > > index = F2FS_BYTES_TO_BLK(range.start); > > pg_end = DIV_ROUND_UP(end_addr, F2FS_BLKSIZE); <= BLKSIZE aligned > > > > Are you worried about the case that sudden power-off occurs while a > > file is being truncated? > > ex) 1GB file is being truncated to 4KB -> sudden power-off -> > > i_size(4KB), i_blocks(maybe somewhere between 4KB and 1GB) > > Yes. Basically, I believe we can have some data beyond i_size like fsverity. > Note that fs-verity files are read-only, and therefore this ioctl can't be used on them (since it requires a writable file descriptor). So that case doesn't need to be handled here. - Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:25 ` Eric Biggers @ 2020-07-10 3:29 ` Jaegeuk Kim 0 siblings, 0 replies; 14+ messages in thread From: Jaegeuk Kim @ 2020-07-10 3:29 UTC (permalink / raw) To: Eric Biggers Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 07/09, Eric Biggers wrote: > On Thu, Jul 09, 2020 at 08:20:35PM -0700, Jaegeuk Kim wrote: > > On 07/10, Daeho Jeong wrote: > > > 1. The valid data will be within i_size. > > > 2. All the trim operations will be done in a unit of block, even if > > > i_size is not aligned with BLKSIZE like the below. > > > > > > index = F2FS_BYTES_TO_BLK(range.start); > > > pg_end = DIV_ROUND_UP(end_addr, F2FS_BLKSIZE); <= BLKSIZE aligned > > > > > > Are you worried about the case that sudden power-off occurs while a > > > file is being truncated? > > > ex) 1GB file is being truncated to 4KB -> sudden power-off -> > > > i_size(4KB), i_blocks(maybe somewhere between 4KB and 1GB) > > > > Yes. Basically, I believe we can have some data beyond i_size like fsverity. > > > > Note that fs-verity files are read-only, and therefore this ioctl can't be used > on them (since it requires a writable file descriptor). So that case doesn't > need to be handled here. I meant it as an example of valid data beyond i_size. > > - Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:02 ` [f2fs-dev] " Jaegeuk Kim 2020-07-10 3:17 ` Daeho Jeong @ 2020-07-10 3:21 ` Chao Yu 2020-07-10 3:31 ` Jaegeuk Kim 1 sibling, 1 reply; 14+ messages in thread From: Chao Yu @ 2020-07-10 3:21 UTC (permalink / raw) To: Jaegeuk Kim, Daeho Jeong Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 2020/7/10 11:02, Jaegeuk Kim wrote: > On 07/10, Daeho Jeong wrote: >> From: Daeho Jeong <daehojeong@google.com> >> >> Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. >> 1. Added -1 value support for range.len to signify the end of file. >> 2. If the end of the range passes over the end of file, it means until >> the end of file. >> 3. ignored the case of that range.len is zero to prevent the function >> from making end_addr zero and triggering different behaviour of >> the function. >> >> Signed-off-by: Daeho Jeong <daehojeong@google.com> >> --- >> fs/f2fs/file.c | 16 +++++++--------- >> 1 file changed, 7 insertions(+), 9 deletions(-) >> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >> index 368c80f8e2a1..1c4601f99326 100644 >> --- a/fs/f2fs/file.c >> +++ b/fs/f2fs/file.c >> @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) >> file_start_write(filp); >> inode_lock(inode); >> >> - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { >> + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || >> + range.start >= inode->i_size) { >> ret = -EINVAL; >> goto err; >> } >> >> - if (range.start >= inode->i_size) { >> - ret = -EINVAL; >> + if (range.len == 0) >> goto err; >> - } >> >> - if (inode->i_size - range.start < range.len) { >> - ret = -E2BIG; >> - goto err; >> - } >> - end_addr = range.start + range.len; >> + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) >> + end_addr = inode->i_size; We can remove 'range.len == (u64)-1' condition since later condition can cover this? > > Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for The blocks beyond i_size will never be written, there won't be any valid message there, so we don't need to worry about that. Thanks, > ending criteria? > >> + else >> + end_addr = range.start + range.len; >> >> to_end = (end_addr == inode->i_size); >> if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || >> -- >> 2.27.0.383.g050319c2ae-goog >> >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:21 ` Chao Yu @ 2020-07-10 3:31 ` Jaegeuk Kim 2020-07-10 3:36 ` Chao Yu 0 siblings, 1 reply; 14+ messages in thread From: Jaegeuk Kim @ 2020-07-10 3:31 UTC (permalink / raw) To: Chao Yu Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 07/10, Chao Yu wrote: > On 2020/7/10 11:02, Jaegeuk Kim wrote: > > On 07/10, Daeho Jeong wrote: > >> From: Daeho Jeong <daehojeong@google.com> > >> > >> Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. > >> 1. Added -1 value support for range.len to signify the end of file. > >> 2. If the end of the range passes over the end of file, it means until > >> the end of file. > >> 3. ignored the case of that range.len is zero to prevent the function > >> from making end_addr zero and triggering different behaviour of > >> the function. > >> > >> Signed-off-by: Daeho Jeong <daehojeong@google.com> > >> --- > >> fs/f2fs/file.c | 16 +++++++--------- > >> 1 file changed, 7 insertions(+), 9 deletions(-) > >> > >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >> index 368c80f8e2a1..1c4601f99326 100644 > >> --- a/fs/f2fs/file.c > >> +++ b/fs/f2fs/file.c > >> @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) > >> file_start_write(filp); > >> inode_lock(inode); > >> > >> - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { > >> + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || > >> + range.start >= inode->i_size) { > >> ret = -EINVAL; > >> goto err; > >> } > >> > >> - if (range.start >= inode->i_size) { > >> - ret = -EINVAL; > >> + if (range.len == 0) > >> goto err; > >> - } > >> > >> - if (inode->i_size - range.start < range.len) { > >> - ret = -E2BIG; > >> - goto err; > >> - } > >> - end_addr = range.start + range.len; > >> + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) > >> + end_addr = inode->i_size; > > We can remove 'range.len == (u64)-1' condition since later condition can cover > this? > > > > > Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for > > The blocks beyond i_size will never be written, there won't be any valid message > there, so we don't need to worry about that. I don't think we have a way to guarantee the order of i_size and block allocation in f2fs. See f2fs_write_begin and f2fs_write_end. > > Thanks, > > > ending criteria? > > > >> + else > >> + end_addr = range.start + range.len; > >> > >> to_end = (end_addr == inode->i_size); > >> if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || > >> -- > >> 2.27.0.383.g050319c2ae-goog > >> > >> > >> > >> _______________________________________________ > >> Linux-f2fs-devel mailing list > >> Linux-f2fs-devel@lists.sourceforge.net > >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > . > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:31 ` Jaegeuk Kim @ 2020-07-10 3:36 ` Chao Yu 2020-07-10 3:52 ` Jaegeuk Kim 0 siblings, 1 reply; 14+ messages in thread From: Chao Yu @ 2020-07-10 3:36 UTC (permalink / raw) To: Jaegeuk Kim Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 2020/7/10 11:31, Jaegeuk Kim wrote: > On 07/10, Chao Yu wrote: >> On 2020/7/10 11:02, Jaegeuk Kim wrote: >>> On 07/10, Daeho Jeong wrote: >>>> From: Daeho Jeong <daehojeong@google.com> >>>> >>>> Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. >>>> 1. Added -1 value support for range.len to signify the end of file. >>>> 2. If the end of the range passes over the end of file, it means until >>>> the end of file. >>>> 3. ignored the case of that range.len is zero to prevent the function >>>> from making end_addr zero and triggering different behaviour of >>>> the function. >>>> >>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>>> --- >>>> fs/f2fs/file.c | 16 +++++++--------- >>>> 1 file changed, 7 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>> index 368c80f8e2a1..1c4601f99326 100644 >>>> --- a/fs/f2fs/file.c >>>> +++ b/fs/f2fs/file.c >>>> @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) >>>> file_start_write(filp); >>>> inode_lock(inode); >>>> >>>> - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { >>>> + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || >>>> + range.start >= inode->i_size) { >>>> ret = -EINVAL; >>>> goto err; >>>> } >>>> >>>> - if (range.start >= inode->i_size) { >>>> - ret = -EINVAL; >>>> + if (range.len == 0) >>>> goto err; >>>> - } >>>> >>>> - if (inode->i_size - range.start < range.len) { >>>> - ret = -E2BIG; >>>> - goto err; >>>> - } >>>> - end_addr = range.start + range.len; >>>> + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) >>>> + end_addr = inode->i_size; >> >> We can remove 'range.len == (u64)-1' condition since later condition can cover >> this? >> >>> >>> Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for >> >> The blocks beyond i_size will never be written, there won't be any valid message >> there, so we don't need to worry about that. > > I don't think we have a way to guarantee the order of i_size and block > allocation in f2fs. See f2fs_write_begin and f2fs_write_end. However, write_begin & write_end are covered by inode_lock, it could not be racy with inode size check in f2fs_sec_trim_file() as it hold inode_lock as well? > >> >> Thanks, >> >>> ending criteria? >>> >>>> + else >>>> + end_addr = range.start + range.len; >>>> >>>> to_end = (end_addr == inode->i_size); >>>> if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || >>>> -- >>>> 2.27.0.383.g050319c2ae-goog >>>> >>>> >>>> >>>> _______________________________________________ >>>> Linux-f2fs-devel mailing list >>>> Linux-f2fs-devel@lists.sourceforge.net >>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>> >>> >>> _______________________________________________ >>> Linux-f2fs-devel mailing list >>> Linux-f2fs-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>> . >>> > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:36 ` Chao Yu @ 2020-07-10 3:52 ` Jaegeuk Kim 2020-07-10 4:26 ` Daeho Jeong 2020-07-10 6:31 ` Chao Yu 0 siblings, 2 replies; 14+ messages in thread From: Jaegeuk Kim @ 2020-07-10 3:52 UTC (permalink / raw) To: Chao Yu Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 07/10, Chao Yu wrote: > On 2020/7/10 11:31, Jaegeuk Kim wrote: > > On 07/10, Chao Yu wrote: > >> On 2020/7/10 11:02, Jaegeuk Kim wrote: > >>> On 07/10, Daeho Jeong wrote: > >>>> From: Daeho Jeong <daehojeong@google.com> > >>>> > >>>> Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. > >>>> 1. Added -1 value support for range.len to signify the end of file. > >>>> 2. If the end of the range passes over the end of file, it means until > >>>> the end of file. > >>>> 3. ignored the case of that range.len is zero to prevent the function > >>>> from making end_addr zero and triggering different behaviour of > >>>> the function. > >>>> > >>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > >>>> --- > >>>> fs/f2fs/file.c | 16 +++++++--------- > >>>> 1 file changed, 7 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>>> index 368c80f8e2a1..1c4601f99326 100644 > >>>> --- a/fs/f2fs/file.c > >>>> +++ b/fs/f2fs/file.c > >>>> @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) > >>>> file_start_write(filp); > >>>> inode_lock(inode); > >>>> > >>>> - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { > >>>> + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || > >>>> + range.start >= inode->i_size) { > >>>> ret = -EINVAL; > >>>> goto err; > >>>> } > >>>> > >>>> - if (range.start >= inode->i_size) { > >>>> - ret = -EINVAL; > >>>> + if (range.len == 0) > >>>> goto err; > >>>> - } > >>>> > >>>> - if (inode->i_size - range.start < range.len) { > >>>> - ret = -E2BIG; > >>>> - goto err; > >>>> - } > >>>> - end_addr = range.start + range.len; > >>>> + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) > >>>> + end_addr = inode->i_size; > >> > >> We can remove 'range.len == (u64)-1' condition since later condition can cover > >> this? > >> > >>> > >>> Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for > >> > >> The blocks beyond i_size will never be written, there won't be any valid message > >> there, so we don't need to worry about that. > > > > I don't think we have a way to guarantee the order of i_size and block > > allocation in f2fs. See f2fs_write_begin and f2fs_write_end. > > However, write_begin & write_end are covered by inode_lock, it could not be > racy with inode size check in f2fs_sec_trim_file() as it hold inode_lock as > well? Like Daeho said, write_begin -> checkpoint -> power-cut can give bigger i_blocks than i_size. > > > > >> > >> Thanks, > >> > >>> ending criteria? > >>> > >>>> + else > >>>> + end_addr = range.start + range.len; > >>>> > >>>> to_end = (end_addr == inode->i_size); > >>>> if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || > >>>> -- > >>>> 2.27.0.383.g050319c2ae-goog > >>>> > >>>> > >>>> > >>>> _______________________________________________ > >>>> Linux-f2fs-devel mailing list > >>>> Linux-f2fs-devel@lists.sourceforge.net > >>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > >>> > >>> > >>> _______________________________________________ > >>> Linux-f2fs-devel mailing list > >>> Linux-f2fs-devel@lists.sourceforge.net > >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > >>> . > >>> > > . > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:52 ` Jaegeuk Kim @ 2020-07-10 4:26 ` Daeho Jeong 2020-07-10 6:31 ` Chao Yu 1 sibling, 0 replies; 14+ messages in thread From: Daeho Jeong @ 2020-07-10 4:26 UTC (permalink / raw) To: Jaegeuk Kim Cc: Chao Yu, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel To handle that case, I think we need to handle range.len(-1) differently. When range.len is -1, we need to find out every block belongs to the inode regardless of i_size and discard it. 2020년 7월 10일 (금) 오후 12:52, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성: > > On 07/10, Chao Yu wrote: > > On 2020/7/10 11:31, Jaegeuk Kim wrote: > > > On 07/10, Chao Yu wrote: > > >> On 2020/7/10 11:02, Jaegeuk Kim wrote: > > >>> On 07/10, Daeho Jeong wrote: > > >>>> From: Daeho Jeong <daehojeong@google.com> > > >>>> > > >>>> Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. > > >>>> 1. Added -1 value support for range.len to signify the end of file. > > >>>> 2. If the end of the range passes over the end of file, it means until > > >>>> the end of file. > > >>>> 3. ignored the case of that range.len is zero to prevent the function > > >>>> from making end_addr zero and triggering different behaviour of > > >>>> the function. > > >>>> > > >>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > > >>>> --- > > >>>> fs/f2fs/file.c | 16 +++++++--------- > > >>>> 1 file changed, 7 insertions(+), 9 deletions(-) > > >>>> > > >>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > >>>> index 368c80f8e2a1..1c4601f99326 100644 > > >>>> --- a/fs/f2fs/file.c > > >>>> +++ b/fs/f2fs/file.c > > >>>> @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) > > >>>> file_start_write(filp); > > >>>> inode_lock(inode); > > >>>> > > >>>> - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { > > >>>> + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || > > >>>> + range.start >= inode->i_size) { > > >>>> ret = -EINVAL; > > >>>> goto err; > > >>>> } > > >>>> > > >>>> - if (range.start >= inode->i_size) { > > >>>> - ret = -EINVAL; > > >>>> + if (range.len == 0) > > >>>> goto err; > > >>>> - } > > >>>> > > >>>> - if (inode->i_size - range.start < range.len) { > > >>>> - ret = -E2BIG; > > >>>> - goto err; > > >>>> - } > > >>>> - end_addr = range.start + range.len; > > >>>> + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) > > >>>> + end_addr = inode->i_size; > > >> > > >> We can remove 'range.len == (u64)-1' condition since later condition can cover > > >> this? > > >> > > >>> > > >>> Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for > > >> > > >> The blocks beyond i_size will never be written, there won't be any valid message > > >> there, so we don't need to worry about that. > > > > > > I don't think we have a way to guarantee the order of i_size and block > > > allocation in f2fs. See f2fs_write_begin and f2fs_write_end. > > > > However, write_begin & write_end are covered by inode_lock, it could not be > > racy with inode size check in f2fs_sec_trim_file() as it hold inode_lock as > > well? > > Like Daeho said, write_begin -> checkpoint -> power-cut can give bigger i_blocks > than i_size. > > > > > > > > >> > > >> Thanks, > > >> > > >>> ending criteria? > > >>> > > >>>> + else > > >>>> + end_addr = range.start + range.len; > > >>>> > > >>>> to_end = (end_addr == inode->i_size); > > >>>> if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || > > >>>> -- > > >>>> 2.27.0.383.g050319c2ae-goog > > >>>> > > >>>> > > >>>> > > >>>> _______________________________________________ > > >>>> Linux-f2fs-devel mailing list > > >>>> Linux-f2fs-devel@lists.sourceforge.net > > >>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > >>> > > >>> > > >>> _______________________________________________ > > >>> Linux-f2fs-devel mailing list > > >>> Linux-f2fs-devel@lists.sourceforge.net > > >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > >>> . > > >>> > > > . > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 3:52 ` Jaegeuk Kim 2020-07-10 4:26 ` Daeho Jeong @ 2020-07-10 6:31 ` Chao Yu 2020-07-10 6:46 ` Jaegeuk Kim 1 sibling, 1 reply; 14+ messages in thread From: Chao Yu @ 2020-07-10 6:31 UTC (permalink / raw) To: Jaegeuk Kim Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 2020/7/10 11:52, Jaegeuk Kim wrote: > On 07/10, Chao Yu wrote: >> On 2020/7/10 11:31, Jaegeuk Kim wrote: >>> On 07/10, Chao Yu wrote: >>>> On 2020/7/10 11:02, Jaegeuk Kim wrote: >>>>> On 07/10, Daeho Jeong wrote: >>>>>> From: Daeho Jeong <daehojeong@google.com> >>>>>> >>>>>> Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. >>>>>> 1. Added -1 value support for range.len to signify the end of file. >>>>>> 2. If the end of the range passes over the end of file, it means until >>>>>> the end of file. >>>>>> 3. ignored the case of that range.len is zero to prevent the function >>>>>> from making end_addr zero and triggering different behaviour of >>>>>> the function. >>>>>> >>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>>>>> --- >>>>>> fs/f2fs/file.c | 16 +++++++--------- >>>>>> 1 file changed, 7 insertions(+), 9 deletions(-) >>>>>> >>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>>>> index 368c80f8e2a1..1c4601f99326 100644 >>>>>> --- a/fs/f2fs/file.c >>>>>> +++ b/fs/f2fs/file.c >>>>>> @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) >>>>>> file_start_write(filp); >>>>>> inode_lock(inode); >>>>>> >>>>>> - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { >>>>>> + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || >>>>>> + range.start >= inode->i_size) { >>>>>> ret = -EINVAL; >>>>>> goto err; >>>>>> } >>>>>> >>>>>> - if (range.start >= inode->i_size) { >>>>>> - ret = -EINVAL; >>>>>> + if (range.len == 0) >>>>>> goto err; >>>>>> - } >>>>>> >>>>>> - if (inode->i_size - range.start < range.len) { >>>>>> - ret = -E2BIG; >>>>>> - goto err; >>>>>> - } >>>>>> - end_addr = range.start + range.len; >>>>>> + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) >>>>>> + end_addr = inode->i_size; >>>> >>>> We can remove 'range.len == (u64)-1' condition since later condition can cover >>>> this? >>>> >>>>> >>>>> Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for >>>> >>>> The blocks beyond i_size will never be written, there won't be any valid message >>>> there, so we don't need to worry about that. >>> >>> I don't think we have a way to guarantee the order of i_size and block >>> allocation in f2fs. See f2fs_write_begin and f2fs_write_end. >> >> However, write_begin & write_end are covered by inode_lock, it could not be >> racy with inode size check in f2fs_sec_trim_file() as it hold inode_lock as >> well? > > Like Daeho said, write_begin -> checkpoint -> power-cut can give bigger i_blocks > than i_size. The path won't, cp only persists reserved block in dnode rather than written data block in segment, because data will be copied to page cache after write_begin. I think truncation path could as Daeho said: 1. truncate -> i_size update however blocks wasn't truncated yet -> checkpoint -> recovery 2. truncate failed -> i_size update however partial blocks was truncated -> fsync > >> >>> >>>> >>>> Thanks, >>>> >>>>> ending criteria? >>>>> >>>>>> + else >>>>>> + end_addr = range.start + range.len; >>>>>> >>>>>> to_end = (end_addr == inode->i_size); >>>>>> if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || >>>>>> -- >>>>>> 2.27.0.383.g050319c2ae-goog >>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> Linux-f2fs-devel mailing list >>>>>> Linux-f2fs-devel@lists.sourceforge.net >>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>>>> >>>>> >>>>> _______________________________________________ >>>>> Linux-f2fs-devel mailing list >>>>> Linux-f2fs-devel@lists.sourceforge.net >>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>>>> . >>>>> >>> . >>> > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 6:31 ` Chao Yu @ 2020-07-10 6:46 ` Jaegeuk Kim 2020-07-10 7:29 ` Chao Yu 0 siblings, 1 reply; 14+ messages in thread From: Jaegeuk Kim @ 2020-07-10 6:46 UTC (permalink / raw) To: Chao Yu Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 07/10, Chao Yu wrote: > On 2020/7/10 11:52, Jaegeuk Kim wrote: > > On 07/10, Chao Yu wrote: > >> On 2020/7/10 11:31, Jaegeuk Kim wrote: > >>> On 07/10, Chao Yu wrote: > >>>> On 2020/7/10 11:02, Jaegeuk Kim wrote: > >>>>> On 07/10, Daeho Jeong wrote: > >>>>>> From: Daeho Jeong <daehojeong@google.com> > >>>>>> > >>>>>> Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. > >>>>>> 1. Added -1 value support for range.len to signify the end of file. > >>>>>> 2. If the end of the range passes over the end of file, it means until > >>>>>> the end of file. > >>>>>> 3. ignored the case of that range.len is zero to prevent the function > >>>>>> from making end_addr zero and triggering different behaviour of > >>>>>> the function. > >>>>>> > >>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > >>>>>> --- > >>>>>> fs/f2fs/file.c | 16 +++++++--------- > >>>>>> 1 file changed, 7 insertions(+), 9 deletions(-) > >>>>>> > >>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>>>>> index 368c80f8e2a1..1c4601f99326 100644 > >>>>>> --- a/fs/f2fs/file.c > >>>>>> +++ b/fs/f2fs/file.c > >>>>>> @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) > >>>>>> file_start_write(filp); > >>>>>> inode_lock(inode); > >>>>>> > >>>>>> - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { > >>>>>> + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || > >>>>>> + range.start >= inode->i_size) { > >>>>>> ret = -EINVAL; > >>>>>> goto err; > >>>>>> } > >>>>>> > >>>>>> - if (range.start >= inode->i_size) { > >>>>>> - ret = -EINVAL; > >>>>>> + if (range.len == 0) > >>>>>> goto err; > >>>>>> - } > >>>>>> > >>>>>> - if (inode->i_size - range.start < range.len) { > >>>>>> - ret = -E2BIG; > >>>>>> - goto err; > >>>>>> - } > >>>>>> - end_addr = range.start + range.len; > >>>>>> + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) > >>>>>> + end_addr = inode->i_size; > >>>> > >>>> We can remove 'range.len == (u64)-1' condition since later condition can cover > >>>> this? > >>>> > >>>>> > >>>>> Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for > >>>> > >>>> The blocks beyond i_size will never be written, there won't be any valid message > >>>> there, so we don't need to worry about that. > >>> > >>> I don't think we have a way to guarantee the order of i_size and block > >>> allocation in f2fs. See f2fs_write_begin and f2fs_write_end. > >> > >> However, write_begin & write_end are covered by inode_lock, it could not be > >> racy with inode size check in f2fs_sec_trim_file() as it hold inode_lock as > >> well? > > > > Like Daeho said, write_begin -> checkpoint -> power-cut can give bigger i_blocks > > than i_size. > > The path won't, cp only persists reserved block in dnode rather than written > data block in segment, because data will be copied to page cache after write_begin. Ah, you're talking about data validity, while I was doing block allocation in this case. In either cases, I'd say secure_trim needs to trim whatever data in valid block *address*. > > I think truncation path could as Daeho said: > > 1. truncate -> i_size update however blocks wasn't truncated yet -> checkpoint -> recovery > 2. truncate failed -> i_size update however partial blocks was truncated -> fsync > > > > >> > >>> > >>>> > >>>> Thanks, > >>>> > >>>>> ending criteria? > >>>>> > >>>>>> + else > >>>>>> + end_addr = range.start + range.len; > >>>>>> > >>>>>> to_end = (end_addr == inode->i_size); > >>>>>> if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || > >>>>>> -- > >>>>>> 2.27.0.383.g050319c2ae-goog > >>>>>> > >>>>>> > >>>>>> > >>>>>> _______________________________________________ > >>>>>> Linux-f2fs-devel mailing list > >>>>>> Linux-f2fs-devel@lists.sourceforge.net > >>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > >>>>> > >>>>> > >>>>> _______________________________________________ > >>>>> Linux-f2fs-devel mailing list > >>>>> Linux-f2fs-devel@lists.sourceforge.net > >>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > >>>>> . > >>>>> > >>> . > >>> > > . > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE 2020-07-10 6:46 ` Jaegeuk Kim @ 2020-07-10 7:29 ` Chao Yu 0 siblings, 0 replies; 14+ messages in thread From: Chao Yu @ 2020-07-10 7:29 UTC (permalink / raw) To: Jaegeuk Kim Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 2020/7/10 14:46, Jaegeuk Kim wrote: > On 07/10, Chao Yu wrote: >> On 2020/7/10 11:52, Jaegeuk Kim wrote: >>> On 07/10, Chao Yu wrote: >>>> On 2020/7/10 11:31, Jaegeuk Kim wrote: >>>>> On 07/10, Chao Yu wrote: >>>>>> On 2020/7/10 11:02, Jaegeuk Kim wrote: >>>>>>> On 07/10, Daeho Jeong wrote: >>>>>>>> From: Daeho Jeong <daehojeong@google.com> >>>>>>>> >>>>>>>> Changed the way of handling range.len of F2FS_IOC_SEC_TRIM_FILE. >>>>>>>> 1. Added -1 value support for range.len to signify the end of file. >>>>>>>> 2. If the end of the range passes over the end of file, it means until >>>>>>>> the end of file. >>>>>>>> 3. ignored the case of that range.len is zero to prevent the function >>>>>>>> from making end_addr zero and triggering different behaviour of >>>>>>>> the function. >>>>>>>> >>>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>>>>>>> --- >>>>>>>> fs/f2fs/file.c | 16 +++++++--------- >>>>>>>> 1 file changed, 7 insertions(+), 9 deletions(-) >>>>>>>> >>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>>>>>> index 368c80f8e2a1..1c4601f99326 100644 >>>>>>>> --- a/fs/f2fs/file.c >>>>>>>> +++ b/fs/f2fs/file.c >>>>>>>> @@ -3813,21 +3813,19 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) >>>>>>>> file_start_write(filp); >>>>>>>> inode_lock(inode); >>>>>>>> >>>>>>>> - if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode)) { >>>>>>>> + if (f2fs_is_atomic_file(inode) || f2fs_compressed_file(inode) || >>>>>>>> + range.start >= inode->i_size) { >>>>>>>> ret = -EINVAL; >>>>>>>> goto err; >>>>>>>> } >>>>>>>> >>>>>>>> - if (range.start >= inode->i_size) { >>>>>>>> - ret = -EINVAL; >>>>>>>> + if (range.len == 0) >>>>>>>> goto err; >>>>>>>> - } >>>>>>>> >>>>>>>> - if (inode->i_size - range.start < range.len) { >>>>>>>> - ret = -E2BIG; >>>>>>>> - goto err; >>>>>>>> - } >>>>>>>> - end_addr = range.start + range.len; >>>>>>>> + if (range.len == (u64)-1 || inode->i_size - range.start < range.len) >>>>>>>> + end_addr = inode->i_size; >>>>>> >>>>>> We can remove 'range.len == (u64)-1' condition since later condition can cover >>>>>> this? >>>>>> >>>>>>> >>>>>>> Hmm, what if there are blocks beyond i_size? Do we need to check i_blocks for >>>>>> >>>>>> The blocks beyond i_size will never be written, there won't be any valid message >>>>>> there, so we don't need to worry about that. >>>>> >>>>> I don't think we have a way to guarantee the order of i_size and block >>>>> allocation in f2fs. See f2fs_write_begin and f2fs_write_end. >>>> >>>> However, write_begin & write_end are covered by inode_lock, it could not be >>>> racy with inode size check in f2fs_sec_trim_file() as it hold inode_lock as >>>> well? >>> >>> Like Daeho said, write_begin -> checkpoint -> power-cut can give bigger i_blocks >>> than i_size. >> >> The path won't, cp only persists reserved block in dnode rather than written >> data block in segment, because data will be copied to page cache after write_begin. > > Ah, you're talking about data validity, while I was doing block allocation in > this case. In either cases, I'd say secure_trim needs to trim whatever data > in valid block *address*. Yeah, I agreed, sec_trim should trim all data no matter locating inside or beyond isize. > >> >> I think truncation path could as Daeho said: >> >> 1. truncate -> i_size update however blocks wasn't truncated yet -> checkpoint -> recovery >> 2. truncate failed -> i_size update however partial blocks was truncated -> fsync >> >>> >>>> >>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>>> ending criteria? >>>>>>> >>>>>>>> + else >>>>>>>> + end_addr = range.start + range.len; >>>>>>>> >>>>>>>> to_end = (end_addr == inode->i_size); >>>>>>>> if (!IS_ALIGNED(range.start, F2FS_BLKSIZE) || >>>>>>>> -- >>>>>>>> 2.27.0.383.g050319c2ae-goog >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Linux-f2fs-devel mailing list >>>>>>>> Linux-f2fs-devel@lists.sourceforge.net >>>>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Linux-f2fs-devel mailing list >>>>>>> Linux-f2fs-devel@lists.sourceforge.net >>>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>>>>>> . >>>>>>> >>>>> . >>>>> >>> . >>> > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-07-10 7:30 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-10 2:15 [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE Daeho Jeong 2020-07-10 3:02 ` [f2fs-dev] " Jaegeuk Kim 2020-07-10 3:17 ` Daeho Jeong 2020-07-10 3:20 ` Jaegeuk Kim 2020-07-10 3:25 ` Eric Biggers 2020-07-10 3:29 ` Jaegeuk Kim 2020-07-10 3:21 ` Chao Yu 2020-07-10 3:31 ` Jaegeuk Kim 2020-07-10 3:36 ` Chao Yu 2020-07-10 3:52 ` Jaegeuk Kim 2020-07-10 4:26 ` Daeho Jeong 2020-07-10 6:31 ` Chao Yu 2020-07-10 6:46 ` Jaegeuk Kim 2020-07-10 7:29 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox