From: Daeho Jeong <[email protected]>
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 <[email protected]>
---
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
On 07/10, Daeho Jeong wrote:
> From: Daeho Jeong <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
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 <[email protected]>님이 작성:
>
> On 07/10, Daeho Jeong wrote:
> > From: Daeho Jeong <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
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 <[email protected]>님이 작성:
> >
> > On 07/10, Daeho Jeong wrote:
> > > From: Daeho Jeong <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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
> > > [email protected]
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2020/7/10 11:02, Jaegeuk Kim wrote:
> On 07/10, Daeho Jeong wrote:
>> From: Daeho Jeong <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>
On Thu, Jul 09, 2020 at 08:20:35PM -0700, Jaegeuk Kim wrote:
> On 07/10, Daeho Jeong wrote:
> > 1. The valid data will be within i_size.
> > 2. All the trim operations will be done in a unit of block, even if
> > i_size is not aligned with BLKSIZE like the below.
> >
> > index = F2FS_BYTES_TO_BLK(range.start);
> > pg_end = DIV_ROUND_UP(end_addr, F2FS_BLKSIZE); <= BLKSIZE aligned
> >
> > Are you worried about the case that sudden power-off occurs while a
> > file is being truncated?
> > ex) 1GB file is being truncated to 4KB -> sudden power-off ->
> > i_size(4KB), i_blocks(maybe somewhere between 4KB and 1GB)
>
> Yes. Basically, I believe we can have some data beyond i_size like fsverity.
>
Note that fs-verity files are read-only, and therefore this ioctl can't be used
on them (since it requires a writable file descriptor). So that case doesn't
need to be handled here.
- Eric
On 07/10, Chao Yu wrote:
> On 2020/7/10 11:02, Jaegeuk Kim wrote:
> > On 07/10, Daeho Jeong wrote:
> >> From: Daeho Jeong <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> 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
> >> [email protected]
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
> >
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > .
> >
On 07/09, Eric Biggers wrote:
> On Thu, Jul 09, 2020 at 08:20:35PM -0700, Jaegeuk Kim wrote:
> > On 07/10, Daeho Jeong wrote:
> > > 1. The valid data will be within i_size.
> > > 2. All the trim operations will be done in a unit of block, even if
> > > i_size is not aligned with BLKSIZE like the below.
> > >
> > > index = F2FS_BYTES_TO_BLK(range.start);
> > > pg_end = DIV_ROUND_UP(end_addr, F2FS_BLKSIZE); <= BLKSIZE aligned
> > >
> > > Are you worried about the case that sudden power-off occurs while a
> > > file is being truncated?
> > > ex) 1GB file is being truncated to 4KB -> sudden power-off ->
> > > i_size(4KB), i_blocks(maybe somewhere between 4KB and 1GB)
> >
> > Yes. Basically, I believe we can have some data beyond i_size like fsverity.
> >
>
> Note that fs-verity files are read-only, and therefore this ioctl can't be used
> on them (since it requires a writable file descriptor). So that case doesn't
> need to be handled here.
I meant it as an example of valid data beyond i_size.
>
> - Eric
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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
>>>> [email protected]
>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>> .
>>>
> .
>
On 07/10, Chao Yu wrote:
> On 2020/7/10 11:31, Jaegeuk Kim wrote:
> > On 07/10, Chao Yu wrote:
> >> On 2020/7/10 11:02, Jaegeuk Kim wrote:
> >>> On 07/10, Daeho Jeong wrote:
> >>>> From: Daeho Jeong <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >>>> ---
> >>>> 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
> >>>> [email protected]
> >>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>
> >>>
> >>> _______________________________________________
> >>> Linux-f2fs-devel mailing list
> >>> [email protected]
> >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>> .
> >>>
> > .
> >
To handle that case, I think we need to handle range.len(-1) differently.
When range.len is -1, we need to find out every block belongs to the
inode regardless of i_size and discard it.
2020년 7월 10일 (금) 오후 12:52, Jaegeuk Kim <[email protected]>님이 작성:
>
> 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 <[email protected]>
> > >>>>
> > >>>> 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 <[email protected]>
> > >>>> ---
> > >>>> 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
> > >>>> [email protected]
> > >>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > >>>
> > >>>
> > >>> _______________________________________________
> > >>> Linux-f2fs-devel mailing list
> > >>> [email protected]
> > >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > >>> .
> > >>>
> > > .
> > >
On 2020/7/10 11:52, Jaegeuk Kim wrote:
> On 07/10, Chao Yu wrote:
>> On 2020/7/10 11:31, Jaegeuk Kim wrote:
>>> On 07/10, Chao Yu wrote:
>>>> On 2020/7/10 11:02, Jaegeuk Kim wrote:
>>>>> On 07/10, Daeho Jeong wrote:
>>>>>> From: Daeho Jeong <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
>>>>>> [email protected]
>>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Linux-f2fs-devel mailing list
>>>>> [email protected]
>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>> .
>>>>>
>>> .
>>>
> .
>
On 07/10, Chao Yu wrote:
> On 2020/7/10 11:52, Jaegeuk Kim wrote:
> > On 07/10, Chao Yu wrote:
> >> On 2020/7/10 11:31, Jaegeuk Kim wrote:
> >>> On 07/10, Chao Yu wrote:
> >>>> On 2020/7/10 11:02, Jaegeuk Kim wrote:
> >>>>> On 07/10, Daeho Jeong wrote:
> >>>>>> From: Daeho Jeong <[email protected]>
> >>>>>>
> >>>>>> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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
> >>>>>> [email protected]
> >>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> Linux-f2fs-devel mailing list
> >>>>> [email protected]
> >>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>>> .
> >>>>>
> >>> .
> >>>
> > .
> >
On 2020/7/10 14:46, Jaegeuk Kim wrote:
> On 07/10, Chao Yu wrote:
>> On 2020/7/10 11:52, Jaegeuk Kim wrote:
>>> On 07/10, Chao Yu wrote:
>>>> On 2020/7/10 11:31, Jaegeuk Kim wrote:
>>>>> On 07/10, Chao Yu wrote:
>>>>>> On 2020/7/10 11:02, Jaegeuk Kim wrote:
>>>>>>> On 07/10, Daeho Jeong wrote:
>>>>>>>> From: Daeho Jeong <[email protected]>
>>>>>>>>
>>>>>>>> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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
>>>>>>>> [email protected]
>>>>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Linux-f2fs-devel mailing list
>>>>>>> [email protected]
>>>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>>>> .
>>>>>>>
>>>>> .
>>>>>
>>> .
>>>
> .
>