[f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[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



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[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

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[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

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Eric Biggers]

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Eric Biggers]

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

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Chao Yu]

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
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Chao Yu]

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
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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
> > .
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Chao Yu]

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
>>> .
>>>
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Chao Yu]

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
>>> .
>>>
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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
> >>> .
> >>>
> > .
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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
> >>> .
> >>>
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Daeho Jeong]

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
> > >>> .
> > >>>
> > > .
> > >


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Daeho Jeong]

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
> > >>> .
> > >>>
> > > .
> > >

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Chao Yu]

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
>>>>> .
>>>>>
>>> .
>>>
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Chao Yu]

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
>>>>> .
>>>>>
>>> .
>>>
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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
> >>>>> .
> >>>>>
> >>> .
> >>>
> > .
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Jaegeuk Kim]

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
> >>>>> .
> >>>>>
> >>> .
> >>>
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Chao Yu]

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
>>>>>>> .
>>>>>>>
>>>>> .
>>>>>
>>> .
>>>
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: change the way of handling range.len in F2FS_IOC_SEC_TRIM_FILE

[Chao Yu]

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
>>>>>>> .
>>>>>>>
>>>>> .
>>>>>
>>> .
>>>
> .
>