On 2018/3/30 23:39, Jaegeuk Kim wrote:
> On 03/30, Junling Zheng wrote:
>> On 2018/3/30 19:26, Chao Yu wrote:
>>> On 2018/3/30 18:51, Junling Zheng wrote:
>>>> Hi,
>>>>
>>>> On 2018/3/30 17:28, Chao Yu wrote:
>>>>> Hi All,
>>>>>
>>>>> On 2018/3/28 1:19, Jaegeuk Kim wrote:
>>>>>> From: katao <ka...@xiaomi.com>
>>>>>>
>>>>>> The args of wanted_total_sectors is calculated based
>>>>>> on the DEFAULT_SECTOR_SIZE(512Bytes).get_device_info(i)
>>>>>> may be reset dev_sector_size, we should reset the number
>>>>>> of wanted_total_sectors.
>>>>>>
>>>>>> This bug was reported to Google Issue Tracker.
>>>>>> Link: https://issuetracker.google.com/issues/76407663
>>>>>
>>>>> I don't think this is the right way, since now we have changed previous
>>>>> sector_counter's meaning, some applications, for example, like xfstests
>>>>> will get
>>>>> device's real sector size via blockdev --getsize64, then calculate total
>>>>> wanted
>>>>> sector count by total_wanted_size / real_sector_size, if we changed
>>>>> default
>>>>> sector size to 512bytes, xfstests will pass a wrong sector number, result
>>>>> in
>>>>> getting wrong partition size.
>>>>>
>>>>> For something worse, in order to get the correct sector number, we have to
>>>>> change the way of calculation method of xfstests for new mkfs, but how can
>>>>> xfstests know the current version of mkfs is new or old...
>>>>>
>>>>> I think the change didn't consider backward compatibility of mkfs, so, in
>>>>> order
>>>>> to keep that, we'd better to let user pass the right sector number based
>>>>> on
>>>>> their device, or we can introduce a new parameter to indicate user wanted
>>>>> total
>>>>> size.
>>>>>
>>>>> How do you think?
>>>>>
>>>>
>>>> Agree. It's not backward-compatible. Most users can pass the correct
>>>> sector number
>>>> calculated by the real sector size. For those very few users using 512B
>>>> despite of
>>>> the actual sector size, all we need to do is informing them the real
>>>> sector size.
>>>
>>> The problem is via passed sector number, we can't know user has already
>>> knew the
>>> real sector size or not, so we don't have any chance to info them.
>>>
>>
>> Yeah, we can't guess users' behaviors. And only when wanted size is over
>> device size,
>> we can inform users the real sector size.
>
> So, current problem would be that wanted_total_sectors is given by:
> - device sector size in xfstests
> - 4KB in fs_mgr in Android
> - 512B in recovery in Android
>
> And, my concern is why user always needs to think about sector_size to give
> target image size, and I thought 512B would be good to set as a default
> smallest
> value.
Of course, IMO, the most direct way is let user pass image size to mkfs.f2fs
instead of caculated sector size, I think that will make all happy.
>
> Setting it 512B by default can give some confusion to users tho, it won't hurt
> the existing partitions or images. So, I bet users will get noticed quickly,
> when formatting new partition under this rule.
For those f2fs users, who makes image based on non-512B sector device, once they
upgrade mkfs.f2fs, they will encounter this issue, that may make bad reputation.
For interface in f2fs private ioctl, sysfs entry, or syscall, we keep the rule
that once we want to do some change on it, we will not change the old interface
directly, instead, introduce a new one to keep backward compatibility of old
one. Still, I hope we can keep that rule in mkfs.f2fs parameter.
>
> For xfstests, we're able to add another testcase in /f2fs to check this is
> applied or not by simply testing some wanted_total_sectors numbers on
> different sector sizes.
But, for people who upgrade mkfs but not the newly introduced testcase will
encounter this issue.
I just make xfstests as an example, I doubt there will be more user/application
on phone/server/pc scenario may be impacted by this.
Thanks,
>
> Thanks,
>
>>
>>> Thanks,
>>>
>>>>
>>>> Thanks,
>>>> Junling
>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>> Signed-off-by: katao <ka...@xiaomi.com>
>>>>>> Signed-off-by: Jaegeuk Kim <jaeg...@google.com>
>>>>>> ---
>>>>>> lib/libf2fs.c | 9 ++++++++-
>>>>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>>>>>> index 0c684d5..5f11796 100644
>>>>>> --- a/lib/libf2fs.c
>>>>>> +++ b/lib/libf2fs.c
>>>>>> @@ -799,8 +799,15 @@ int get_device_info(int i)
>>>>>> #ifdef BLKSSZGET
>>>>>> if (ioctl(fd, BLKSSZGET, §or_size) < 0)
>>>>>> MSG(0, "\tError: Using the default sector
>>>>>> size\n");
>>>>>> - else if (dev->sector_size < sector_size)
>>>>>> + else if (dev->sector_size < sector_size){
>>>>>> + /*
>>>>>> + * wanted_total_sectors need to be reset by new
>>>>>> + * sector_size.
>>>>>> + */
>>>>>> + c.wanted_total_sectors =
>>>>>> (c.wanted_total_sectors *
>>>>>> + dev->sector_size) /
>>>>>> sector_size;
>>>>>> dev->sector_size = sector_size;
>>>>>> + }
>>>>>> #endif
>>>>>> #ifdef BLKGETSIZE64
>>>>>> if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {
>>>>>>
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>> .
>>>
>>
>
> .
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
