> On 7 Aug 2019, at 4:55 PM, Qu Wenruo <quwenruo.bt...@gmx.com> wrote:
>
>
>
> On 2019/8/7 下午4:44, Filipe Manana wrote:
>> On Wed, Aug 7, 2019 at 9:35 AM Anand Jain <anand.j...@oracle.com> wrote:
>>>
>>> Commit 6ba9fc8e628b (btrfs: Ensure btrfs_trim_fs can trim the whole
>>> filesystem) makes sure we always trim starting from the first block group.
>>> However it also removed the range.start validity check which is set in the
>>> context of the user, where its range is from 0 to maximum of filesystem
>>> totalbytes and so we have to check its validity in the kernel.
>>>
>>> Also as in the fstrim(8) [1] the kernel layers may modify the trim range.
>>>
>>> [1]
>>> Further, the kernel block layer reserves the right to adjust the discard
>>> ranges to fit raid stripe geometry, non-trim capable devices in a LVM
>>> setup, etc. These reductions would not be reflected in fstrim_range.len
>>> (the --length option).
>>>
>>> This patch undos the deleted range::start validity check.
>>>
>>> Fixes: 6ba9fc8e628b (btrfs: Ensure btrfs_trim_fs can trim the whole
>>> filesystem)
>>> Signed-off-by: Anand Jain <anand.j...@oracle.com>
>>> ---
>>> With this patch fstests generic/260 is successful now.
>>>
>>> fs/btrfs/ioctl.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index b431f7877e88..9345fcdf80c7 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -521,6 +521,8 @@ static noinline int btrfs_ioctl_fitrim(struct file
>>> *file, void __user *arg)
>>> return -EOPNOTSUPP;
>>> if (copy_from_user(&range, arg, sizeof(range)))
>>> return -EFAULT;
>>> + if (range.start > btrfs_super_total_bytes(fs_info->super_copy))
>>> + return -EINVAL;
>>
>> This makes it impossible to trim block groups that start at an offset
>> greater then the super_total_bytes values.
>
what did I miss? As there is no limit on the range::length
so we can still trim beyond super_total_bytes. So I don’t
understand why not? More below.
> Exactly.
>
>> In fact, in extreme cases
>> it's possible all block groups start at offsets larger then
>> super_total_bytes.
>> Have you considered that, or am I missing something?
>
> And, I have already mentioned exactly the same reason in that commit
> message.
>
> To address it once again, the bytenr is btrfs logical address space, has
> nothing to do with any device.
> Thus it can be [0, U64MAX].
>
Fundamentally, logical address space has no relevance in the user context,
so also I don’t understand your view on how anyone shall use the range::start
even if there is no check?
As in the man page it's ok to adjust the range internally, and as length
can be up to U64MAX we can still trim beyond super_total_bytes?
Thanks, Anand
> Thanks,
> Qu
>
>>
>> The change log is also vague to me, doesn't explain why you are
>> re-adding that check.
>>
>> Thanks.
>>
>>>
>>> /*
>>> * NOTE: Don't truncate the range using super->total_bytes. Bytenr
>>> of
>>> --
>>> 2.21.0 (Apple Git-120)
>>>
>>
>>
>
