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

2020-07-10 Thread 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 

 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 
 ---
  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-de...@lists.sourceforge.net
 https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>
>>>
>>> ___
>>> Linux-f2fs-devel mailing list
>>> linux-f2fs-de...@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

2020-07-10 Thread 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 
> >>
> >> 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 
> >> ---
> >>  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-de...@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
> >
> > ___
> > Linux-f2fs-devel mailing list
> > linux-f2fs-de...@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

2020-07-10 Thread 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 
>>
>> 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 
>> ---
>>  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-de...@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
>
> ___
> Linux-f2fs-devel mailing list
> linux-f2fs-de...@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

2020-07-09 Thread 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 님이 작성:
>
> 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 
> > 
> >  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 
> >  ---
> >   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-de...@lists.sourceforge.net
> >  https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > >>>
> > >>>
> > >>> ___
> > >>> Linux-f2fs-devel mailing list
> > >>> linux-f2fs-de...@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

2020-07-09 Thread 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 
> 
>  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 
>  ---
>   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-de...@lists.sourceforge.net
>  https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>
> >>>
> >>> ___
> >>> Linux-f2fs-devel mailing list
> >>> linux-f2fs-de...@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

2020-07-09 Thread 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 

 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 
 ---
  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-de...@lists.sourceforge.net
 https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>
>>>
>>> ___
>>> Linux-f2fs-devel mailing list
>>> linux-f2fs-de...@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

2020-07-09 Thread 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 
> >>
> >> 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 
> >> ---
> >>  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-de...@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > 
> > 
> > ___
> > Linux-f2fs-devel mailing list
> > linux-f2fs-de...@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

2020-07-09 Thread 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

2020-07-09 Thread 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

2020-07-09 Thread Chao Yu 
On 2020/7/10 11:02, Jaegeuk Kim wrote:
> On 07/10, Daeho Jeong wrote:
>> From: Daeho Jeong 
>>
>> 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 
>> ---
>>  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-de...@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> 
> ___
> Linux-f2fs-devel mailing list
> linux-f2fs-de...@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

2020-07-09 Thread 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 님이 작성:
> >
> > On 07/10, Daeho Jeong wrote:
> > > From: Daeho Jeong 
> > >
> > > 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 
> > > ---
> > >  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-de...@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

2020-07-09 Thread 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 님이 작성:
>
> On 07/10, Daeho Jeong wrote:
> > From: Daeho Jeong 
> >
> > 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 
> > ---
> >  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-de...@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

2020-07-09 Thread Jaegeuk Kim 
On 07/10, Daeho Jeong wrote:
> From: Daeho Jeong 
> 
> 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 
> ---
>  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-de...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel