[PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Jaegeuk Kim]

From: katao <katao@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

Signed-off-by: katao <katao@xiaomi.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@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, &sector_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) {
-- 
2.15.0.531.g2ccb3012c9-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Junling Zheng]

Hi, Jaegeuk:

On 2018/3/28 1:19, Jaegeuk Kim wrote:
> From: katao <katao@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
> 

This fix is puzzling...

IMO, we should not recalculate wanted_total_sectors here.
c.wanted_total_sectors is got from user in f2fs_parse_options.
Users must know the sector size of device they use, and they need to
pass a correct wanted_total_sectors to mkfs.f2fs.

Thanks,
Junling

> Signed-off-by: katao <katao@xiaomi.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@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, &sector_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

[RFC PATCH] mkfs.f2fs: use 512B as the sector size criterion

[Junling Zheng]

Use 512 bytes as the sector size criterion while specifying the
amount of sectors passed to mkfs.

Signed-off-by: Junling Zheng <zhengjunling@huawei.com>
---
 lib/libf2fs.c           | 11 ++++++++---
 mkfs/f2fs_format_main.c |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index 5f11796..5fdcdde 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -1002,9 +1002,14 @@ int f2fs_get_device_info(void)
 		if (get_device_info(i))
 			return -1;
 
-	if (c.wanted_total_sectors < c.total_sectors) {
-		MSG(0, "Info: total device sectors = %"PRIu64" (in %u bytes)\n",
-				c.total_sectors, c.sector_size);
+	if (c.wanted_total_sectors > c.total_sectors)
+		MSG(0, "Warning: total sectors = %"PRIu64", "
+			"wanted sectors = %"PRIu64", in %u bytes\n",
+			c.total_sectors, c.wanted_total_sectors, c.sector_size);
+	else {
+		MSG(0, "Info: total sectors = %"PRIu64", "
+			"wanted sectors = %"PRIu64", in %u bytes\n",
+			c.total_sectors, c.wanted_total_sectors, c.sector_size);
 		c.total_sectors = c.wanted_total_sectors;
 		c.devices[0].total_sectors = c.total_sectors;
 	}
diff --git a/mkfs/f2fs_format_main.c b/mkfs/f2fs_format_main.c
index f23fd84..71fd7c2 100644
--- a/mkfs/f2fs_format_main.c
+++ b/mkfs/f2fs_format_main.c
@@ -57,7 +57,7 @@ static void mkfs_usage()
 	MSG(0, "  -S sparse mode\n");
 	MSG(0, "  -t 0: nodiscard, 1: discard [default:1]\n");
 	MSG(0, "  -z # of sections per zone [default:1]\n");
-	MSG(0, "sectors: number of sectors. [default: determined by device size]\n");
+	MSG(0, "sectors: number of sectors (in 512 bytes). [default: determined by device size]\n");
 	exit(1);
 }
 
-- 
2.16.2


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [RFC PATCH] mkfs.f2fs: use 512B as the sector size criterion

[Junling Zheng]

Hi Jaegeuk,

On 2018/3/29 4:30, Jaegeuk Kim wrote:
> Hi Junling,
> 
> On Wed, Mar 28, 2018 at 2:04 AM, Junling Zheng <zhengjunling@huawei.com <mailto:zhengjunling@huawei.com>> wrote:
> 
>     Use 512 bytes as the sector size criterion while specifying the
>     amount of sectors passed to mkfs.
> 
>     Signed-off-by: Junling Zheng <zhengjunling@huawei.com <mailto:zhengjunling@huawei.com>>
>     ---
>      lib/libf2fs.c           | 11 ++++++++---
>      mkfs/f2fs_format_main.c |  2 +-
>      2 files changed, 9 insertions(+), 4 deletions(-)
> 
>     diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>     index 5f11796..5fdcdde 100644
>     --- a/lib/libf2fs.c
>     +++ b/lib/libf2fs.c
>     @@ -1002,9 +1002,14 @@ int f2fs_get_device_info(void)
>                     if (get_device_info(i))
>                             return -1;
> 
>     -       if (c.wanted_total_sectors < c.total_sectors) {
>     -               MSG(0, "Info: total device sectors = %"PRIu64" (in %u bytes)\n",
>     -                               c.total_sectors, c.sector_size);
>     +       if (c.wanted_total_sectors > c.total_sectors)
>     +               MSG(0, "Warning: total sectors = %"PRIu64", "
>     +                       "wanted sectors = %"PRIu64", in %u bytes\n",
>     +                       c.total_sectors, c.wanted_total_sectors, c.sector_size);
> 
> 
> This seems not warning message. IMO, it'd be enough to inform just
> wanted size over device size?

This warning will not break mkfs, just a warning to inform user, and mkfs will use c.total_sectors
to format device :)

>  
> 
>     +       else {
>     +               MSG(0, "Info: total sectors = %"PRIu64", "
>     +                       "wanted sectors = %"PRIu64", in %u bytes\n",
>     +                       c.total_sectors, c.wanted_total_sectors, c.sector_size);
>                     c.total_sectors = c.wanted_total_sectors;
>                     c.devices[0].total_sectors = c.total_sectors;
>             }
>     diff --git a/mkfs/f2fs_format_main.c b/mkfs/f2fs_format_main.c
>     index f23fd84..71fd7c2 100644
>     --- a/mkfs/f2fs_format_main.c
>     +++ b/mkfs/f2fs_format_main.c
>     @@ -57,7 +57,7 @@ static void mkfs_usage()
>             MSG(0, "  -S sparse mode\n");
>             MSG(0, "  -t 0: nodiscard, 1: discard [default:1]\n");
>             MSG(0, "  -z # of sections per zone [default:1]\n");
>     -       MSG(0, "sectors: number of sectors. [default: determined by device size]\n");
>     +       MSG(0, "sectors: number of sectors (in 512 bytes). [default: determined by device size]\n");
> 
> 
> We also need to specify this in man page.

OK, I'll add it.

>  
> 
>             exit(1);
>      }
> 
>     --
>     2.16.2
> 
> 
> 
> 
> -- 
> Jaegeuk Kim



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [RFC PATCH] mkfs.f2fs: use 512B as the sector size criterion

[Jaegeuk Kim]

On 03/29, Junling Zheng wrote:
> Hi Jaegeuk,
> 
> On 2018/3/29 4:30, Jaegeuk Kim wrote:
> > Hi Junling,
> > 
> > On Wed, Mar 28, 2018 at 2:04 AM, Junling Zheng <zhengjunling@huawei.com <mailto:zhengjunling@huawei.com>> wrote:
> > 
> >     Use 512 bytes as the sector size criterion while specifying the
> >     amount of sectors passed to mkfs.
> > 
> >     Signed-off-by: Junling Zheng <zhengjunling@huawei.com <mailto:zhengjunling@huawei.com>>
> >     ---
> >      lib/libf2fs.c           | 11 ++++++++---
> >      mkfs/f2fs_format_main.c |  2 +-
> >      2 files changed, 9 insertions(+), 4 deletions(-)
> > 
> >     diff --git a/lib/libf2fs.c b/lib/libf2fs.c
> >     index 5f11796..5fdcdde 100644
> >     --- a/lib/libf2fs.c
> >     +++ b/lib/libf2fs.c
> >     @@ -1002,9 +1002,14 @@ int f2fs_get_device_info(void)
> >                     if (get_device_info(i))
> >                             return -1;
> > 
> >     -       if (c.wanted_total_sectors < c.total_sectors) {
> >     -               MSG(0, "Info: total device sectors = %"PRIu64" (in %u bytes)\n",
> >     -                               c.total_sectors, c.sector_size);
> >     +       if (c.wanted_total_sectors > c.total_sectors)
> >     +               MSG(0, "Warning: total sectors = %"PRIu64", "
> >     +                       "wanted sectors = %"PRIu64", in %u bytes\n",
> >     +                       c.total_sectors, c.wanted_total_sectors, c.sector_size);
> > 
> > 
> > This seems not warning message. IMO, it'd be enough to inform just
> > wanted size over device size?
> 
> This warning will not break mkfs, just a warning to inform user, and mkfs will use c.total_sectors
> to format device :)

"Warning:" is warning. :P

> 
> >  
> > 
> >     +       else {
> >     +               MSG(0, "Info: total sectors = %"PRIu64", "
> >     +                       "wanted sectors = %"PRIu64", in %u bytes\n",
> >     +                       c.total_sectors, c.wanted_total_sectors, c.sector_size);
> >                     c.total_sectors = c.wanted_total_sectors;
> >                     c.devices[0].total_sectors = c.total_sectors;
> >             }
> >     diff --git a/mkfs/f2fs_format_main.c b/mkfs/f2fs_format_main.c
> >     index f23fd84..71fd7c2 100644
> >     --- a/mkfs/f2fs_format_main.c
> >     +++ b/mkfs/f2fs_format_main.c
> >     @@ -57,7 +57,7 @@ static void mkfs_usage()
> >             MSG(0, "  -S sparse mode\n");
> >             MSG(0, "  -t 0: nodiscard, 1: discard [default:1]\n");
> >             MSG(0, "  -z # of sections per zone [default:1]\n");
> >     -       MSG(0, "sectors: number of sectors. [default: determined by device size]\n");
> >     +       MSG(0, "sectors: number of sectors (in 512 bytes). [default: determined by device size]\n");
> > 
> > 
> > We also need to specify this in man page.
> 
> OK, I'll add it.
> 
> >  
> > 
> >             exit(1);
> >      }
> > 
> >     --
> >     2.16.2
> > 
> > 
> > 
> > 
> > -- 
> > Jaegeuk Kim
> 
> 
> 
> ------------------------------------------------------------------------------
> 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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[RFC PATCH v2] mkfs.f2fs: use 512B as the sector size criterion

[Junling Zheng]

Use 512 bytes as the sector size criterion while specifying the
amount of sectors passed to mkfs.

Signed-off-by: Junling Zheng <zhengjunling@huawei.com>
---
Changes from v1:
 - specify the sector size criterion in man page
 - replace "Warning" with "Info" while wanted size is over device size

 lib/libf2fs.c           | 5 +++--
 man/mkfs.f2fs.8         | 2 +-
 mkfs/f2fs_format_main.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index 5f11796..906f789 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -1002,9 +1002,10 @@ int f2fs_get_device_info(void)
 		if (get_device_info(i))
 			return -1;
 
+	MSG(0, "Info: total sectors = %"PRIu64", "
+		"wanted sectors = %"PRIu64", in %u bytes\n",
+		c.total_sectors, c.wanted_total_sectors, c.sector_size);
 	if (c.wanted_total_sectors < c.total_sectors) {
-		MSG(0, "Info: total device sectors = %"PRIu64" (in %u bytes)\n",
-				c.total_sectors, c.sector_size);
 		c.total_sectors = c.wanted_total_sectors;
 		c.devices[0].total_sectors = c.total_sectors;
 	}
diff --git a/man/mkfs.f2fs.8 b/man/mkfs.f2fs.8
index c2f9c86..e48ce39 100644
--- a/man/mkfs.f2fs.8
+++ b/man/mkfs.f2fs.8
@@ -63,7 +63,7 @@ mkfs.f2fs \- create an F2FS file system
 is used to create a f2fs file system (usually in a disk partition).
 \fIdevice\fP is the special file corresponding to the device (e.g.
 \fI/dev/sdXX\fP).
-\fIsectors\fP is optionally given for specifing the filesystem size.
+\fIsectors\fP (in 512 bytes) is optionally given for specifing the filesystem size.
 .PP
 The exit code returned by
 .B mkfs.f2fs
diff --git a/mkfs/f2fs_format_main.c b/mkfs/f2fs_format_main.c
index f23fd84..71fd7c2 100644
--- a/mkfs/f2fs_format_main.c
+++ b/mkfs/f2fs_format_main.c
@@ -57,7 +57,7 @@ static void mkfs_usage()
 	MSG(0, "  -S sparse mode\n");
 	MSG(0, "  -t 0: nodiscard, 1: discard [default:1]\n");
 	MSG(0, "  -z # of sections per zone [default:1]\n");
-	MSG(0, "sectors: number of sectors. [default: determined by device size]\n");
+	MSG(0, "sectors: number of sectors (in 512 bytes). [default: determined by device size]\n");
 	exit(1);
 }
 
-- 
2.16.2


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Chao Yu]

Hi All,

On 2018/3/28 1:19, Jaegeuk Kim wrote:
> From: katao <katao@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?

Thanks,

> 
> Signed-off-by: katao <katao@xiaomi.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@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, &sector_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

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Junling Zheng]

Hi,

On 2018/3/30 17:28, Chao Yu wrote:
> Hi All,
> 
> On 2018/3/28 1:19, Jaegeuk Kim wrote:
>> From: katao <katao@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.

Thanks,
Junling

> Thanks,
> 
>>
>> Signed-off-by: katao <katao@xiaomi.com>
>> Signed-off-by: Jaegeuk Kim <jaegeuk@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, &sector_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

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Chao Yu]

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

Thanks,

> 
> Thanks,
> Junling
> 
>> Thanks,
>>
>>>
>>> Signed-off-by: katao <katao@xiaomi.com>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@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, &sector_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

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Junling Zheng]

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

> Thanks,
> 
>>
>> Thanks,
>> Junling
>>
>>> Thanks,
>>>
>>>>
>>>> Signed-off-by: katao <katao@xiaomi.com>
>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@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, &sector_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

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Jaegeuk Kim]

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

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

Thanks,

> 
> > Thanks,
> > 
> >>
> >> Thanks,
> >> Junling
> >>
> >>> Thanks,
> >>>
> >>>>
> >>>> Signed-off-by: katao <katao@xiaomi.com>
> >>>> Signed-off-by: Jaegeuk Kim <jaegeuk@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, &sector_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

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Jaegeuk Kim]

On 03/30, 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 <katao@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.
> 
> 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.

So, I just integrated two patches in terms of this issue. Could you take a
look at this?

>From 9e615dd0a350d336a8067c64d9ef2bb7ce43a3e5 Mon Sep 17 00:00:00 2001
From: katao <katao@xiaomi.com>
Date: Tue, 27 Mar 2018 13:25:46 +0800
Subject: [PATCH] libf2fs,mkfs.f2fs: reset wanted_total_sectors by new
 sector_size

Use 512 bytes as the sector size criterion while specifying the
amount of sectors passed to mkfs.

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

Signed-off-by: katao <katao@xiaomi.com>
Signed-off-by: Junling Zheng <zhengjunling@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
---
 lib/libf2fs.c           | 21 ++++++++++++++++++---
 man/mkfs.f2fs.8         |  2 +-
 mkfs/f2fs_format_main.c |  2 +-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index ffdbccb..af1ca65 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -812,8 +812,21 @@ int get_device_info(int i)
 #ifdef BLKSSZGET
 		if (ioctl(fd, BLKSSZGET, &sector_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.
+			 */
+			if (c.wanted_total_sectors != -1) {
+				c.wanted_total_sectors =
+					(c.wanted_total_sectors *
+						dev->sector_size) / sector_size;
+				MSG(0, "Warn: change wanted sectors = %"PRIu64""
+					" (in %u bytes)\n",
+					c.wanted_total_sectors, sector_size);
+			}
 			dev->sector_size = sector_size;
+		}
 #endif
 #ifdef BLKGETSIZE64
 		if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {
@@ -1008,9 +1021,11 @@ int f2fs_get_device_info(void)
 		if (get_device_info(i))
 			return -1;
 
-	if (c.wanted_total_sectors < c.total_sectors) {
-		MSG(0, "Info: total device sectors = %"PRIu64" (in %u bytes)\n",
+	MSG(0, "Info: total device sectors = %"PRIu64" (in %u bytes)\n",
 				c.total_sectors, c.sector_size);
+	if (c.wanted_total_sectors < c.total_sectors) {
+		MSG(0, "Info: wanted sectors = %"PRIu64" (in 512 bytes)\n",
+				c.wanted_total_sectors);
 		c.total_sectors = c.wanted_total_sectors;
 		c.devices[0].total_sectors = c.total_sectors;
 	}
diff --git a/man/mkfs.f2fs.8 b/man/mkfs.f2fs.8
index c2f9c86..e48ce39 100644
--- a/man/mkfs.f2fs.8
+++ b/man/mkfs.f2fs.8
@@ -63,7 +63,7 @@ mkfs.f2fs \- create an F2FS file system
 is used to create a f2fs file system (usually in a disk partition).
 \fIdevice\fP is the special file corresponding to the device (e.g.
 \fI/dev/sdXX\fP).
-\fIsectors\fP is optionally given for specifing the filesystem size.
+\fIsectors\fP (in 512 bytes) is optionally given for specifing the filesystem size.
 .PP
 The exit code returned by
 .B mkfs.f2fs
diff --git a/mkfs/f2fs_format_main.c b/mkfs/f2fs_format_main.c
index e522b2f..f8361b2 100644
--- a/mkfs/f2fs_format_main.c
+++ b/mkfs/f2fs_format_main.c
@@ -55,7 +55,7 @@ static void mkfs_usage()
 	MSG(0, "  -S sparse mode\n");
 	MSG(0, "  -t 0: nodiscard, 1: discard [default:1]\n");
 	MSG(0, "  -z # of sections per zone [default:1]\n");
-	MSG(0, "sectors: number of sectors. [default: determined by device size]\n");
+	MSG(0, "sectors: number of sectors (in 512 bytes). [default: determined by device size]\n");
 	exit(1);
 }
 
-- 
2.15.0.531.g2ccb3012c9-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Chao Yu]

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 <katao@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 <katao@xiaomi.com>
>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@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, &sector_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

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Jaegeuk Kim]

On 04/02, Chao Yu wrote:
> 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 <katao@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.

No, it can become too large number.

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

Okay, so in order to give more flexible ways, it'd be fine to add
wanted_sector_size by "-w" so that we could blow out all the existing concern
like this.

From: katao <katao@xiaomi.com>
Date: Tue, 27 Mar 2018 13:25:46 +0800
Subject: [PATCH] libf2fs,mkfs.f2fs: add wanted_sector_size for
 wanted_total_sectors

The wanted_total_sectors was determined by device sector size, but sometimes
we don't know precise sector_size by default. So, let's give wanted_sector_size
in such the ambiguous situation.

Signed-off-by: katao <katao@xiaomi.com>
Signed-off-by: Junling Zheng <zhengjunling@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
---
 include/f2fs_fs.h       |  1 +
 lib/libf2fs.c           | 13 +++++++++++++
 man/mkfs.f2fs.8         |  8 ++++++++
 mkfs/f2fs_format_main.c |  6 +++++-
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
index 053ccbe..2d75d39 100644
--- a/include/f2fs_fs.h
+++ b/include/f2fs_fs.h
@@ -335,6 +335,7 @@ struct f2fs_configuration {
 	u_int64_t device_size;
 	u_int64_t total_sectors;
 	u_int64_t wanted_total_sectors;
+	u_int64_t wanted_sector_size;
 	u_int64_t target_sectors;
 	u_int32_t sectors_per_blk;
 	u_int32_t blks_per_seg;
diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index ffdbccb..bb7fe2e 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -593,6 +593,7 @@ void f2fs_init_configuration(void)
 	c.blks_per_seg = DEFAULT_BLOCKS_PER_SEGMENT;
 	c.rootdev_name = get_rootdev();
 	c.wanted_total_sectors = -1;
+	c.wanted_sector_size = -1;
 	c.zoned_mode = 0;
 	c.zoned_model = 0;
 	c.zone_blocks = 0;
@@ -900,6 +901,18 @@ int get_device_info(int i)
 				dev->zone_blocks);
 	}
 #endif
+	/* adjust wanted_total_sectors */
+	if (c.wanted_total_sectors != -1) {
+		MSG(0, "Info: wanted sectors = %"PRIu64" (in %"PRIu64" bytes)\n",
+				c.wanted_total_sectors, c.wanted_sector_size);
+		if (c.wanted_sector_size == -1) {
+			c.wanted_sector_size = dev->sector_size;
+		} else if (dev->sector_size != c.wanted_sector_size) {
+			c.wanted_total_sectors *= c.wanted_sector_size;
+			c.wanted_total_sectors /= dev->sector_size;
+		}
+	}
+
 	c.total_sectors += dev->total_sectors;
 	return 0;
 }
diff --git a/man/mkfs.f2fs.8 b/man/mkfs.f2fs.8
index c2f9c86..442c0ea 100644
--- a/man/mkfs.f2fs.8
+++ b/man/mkfs.f2fs.8
@@ -53,6 +53,10 @@ mkfs.f2fs \- create an F2FS file system
 .I nodiscard/discard
 ]
 [
+.B \-w
+.I specific sector_size for target sectors
+]
+[
 .B \-z
 .I #-of-sections-per-zone
 ]
@@ -125,6 +129,10 @@ Specify 1 or 0 to enable/disable discard policy.
 If the value is equal to 1, discard policy is enabled, otherwise is disable.
 The default value is 1.
 .TP
+.BI \-w "sector-size"
+Specify the sector size in bytes along with given target sectors.
+Without it, the sectors will be calculated by device sector size.
+.TP
 .BI \-z " #-of-sections-per-zone"
 Specify the number of sections per zone. A zone consists of multiple sections.
 F2FS allocates segments for active logs with separated zones as much as possible.
diff --git a/mkfs/f2fs_format_main.c b/mkfs/f2fs_format_main.c
index e522b2f..19f0854 100644
--- a/mkfs/f2fs_format_main.c
+++ b/mkfs/f2fs_format_main.c
@@ -54,6 +54,7 @@ static void mkfs_usage()
 	MSG(0, "  -s # of segments per section [default:1]\n");
 	MSG(0, "  -S sparse mode\n");
 	MSG(0, "  -t 0: nodiscard, 1: discard [default:1]\n");
+	MSG(0, "  -w wanted sector size\n");
 	MSG(0, "  -z # of sections per zone [default:1]\n");
 	MSG(0, "sectors: number of sectors. [default: determined by device size]\n");
 	exit(1);
@@ -102,7 +103,7 @@ static void parse_feature(const char *features)
 
 static void f2fs_parse_options(int argc, char *argv[])
 {
-	static const char *option_string = "qa:c:d:e:l:mo:O:s:S:z:t:f";
+	static const char *option_string = "qa:c:d:e:l:mo:O:s:S:z:t:fw:";
 	int32_t option=0;
 
 	while ((option = getopt(argc,argv,option_string)) != EOF) {
@@ -166,6 +167,9 @@ static void f2fs_parse_options(int argc, char *argv[])
 		case 'f':
 			force_overwrite = 1;
 			break;
+		case 'w':
+			c.wanted_sector_size = atoi(optarg);
+			break;
 		default:
 			MSG(0, "\tError: Unknown option %c\n",option);
 			mkfs_usage();
-- 
2.15.0.531.g2ccb3012c9-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] libf2fs: reset wanted_total_sectors by new sector_size

[Chao Yu]

On 2018/4/3 4:03, Jaegeuk Kim wrote:
> On 04/02, Chao Yu wrote:
>> 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 <katao@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.
> 
> No, it can become too large number.
> 
>>
>>>
>>> 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.
> 
> Okay, so in order to give more flexible ways, it'd be fine to add
> wanted_sector_size by "-w" so that we could blow out all the existing concern
> like this.

That's better.

> 
> From: katao <katao@xiaomi.com>
> Date: Tue, 27 Mar 2018 13:25:46 +0800
> Subject: [PATCH] libf2fs,mkfs.f2fs: add wanted_sector_size for
>  wanted_total_sectors
> 
> The wanted_total_sectors was determined by device sector size, but sometimes
> we don't know precise sector_size by default. So, let's give wanted_sector_size
> in such the ambiguous situation.
> 
> Signed-off-by: katao <katao@xiaomi.com>
> Signed-off-by: Junling Zheng <zhengjunling@huawei.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot