libf2fs: reset wanted_total_sectors by new sector_size

From: katao <[email protected]>

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 <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 lib/libf2fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

lib/libf2fs.c +2 -1

index 0c684d5..5f11796 100644
@@ -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){
+			/*

Hi Junling,

On Wed, Mar 28, 2018 at 2:04 AM, Junling Zheng <[email protected] <mailto:[email protected]>> wrote:

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

Signed-off-by Junling Zheng <[email protected] <mailto:[email protected]>>

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

 

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

 

            exit(1);
     }

    --
    2.16.2

Signature · 1 line

Jaegeuk Kim

Hi Jaegeuk,

Quoted 30 lines · Jaegeuk Kim

Hi Junling,

On Wed, Mar 28, 2018 at 2:04 AM, Junling Zheng <[email protected] <mailto:[email protected]>> wrote:

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

Signed-off-by Junling Zheng <[email protected] <mailto:[email protected]>>

---
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);

Quoted 2 lines

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 :)

Quoted 22 lines

 

    +       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");

Quoted 1 line

We also need to specify this in man page.

OK, I'll add it.

Quoted 13 lines

 

            exit(1);
     }

    --
    2.16.2

Signature · 1 line

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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

From: katao <[email protected]>

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 <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 lib/libf2fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

lib/libf2fs.c +2 -1

index 0c684d5..5f11796 100644
@@ -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){
+			/*

Hi All,

Quoted 9 lines · Jaegeuk Kim

From: katao <[email protected]>

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,

Quoted 24 lines

Signed-off-by katao <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 lib/libf2fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

lib/libf2fs.c +7 -1

index 0c684d5..5f11796 100644
@@ -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;

lib/libf2fs.c int get_device_info(int i) · line 813

Quoted 5 lines

lib/libf2fs.c +1 -0

+		}
 #endif
 #ifdef BLKGETSIZE64
 		if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {

.

Hi,

Quoted 29 lines · Chao Yu

Hi All,

Quoted 9 lines · Jaegeuk Kim

From: katao <[email protected]>

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.

Quoted 2 lines

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

Quoted 26 lines

Thanks,

Quoted 24 lines

Signed-off-by katao <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 lib/libf2fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

lib/libf2fs.c +7 -1

index 0c684d5..5f11796 100644
@@ -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;

Quoted 9 lines

lib/libf2fs.c int get_device_info(int i) · line 813

Quoted 5 lines

lib/libf2fs.c +1 -0

+		}
 #endif
 #ifdef BLKGETSIZE64
 		if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {

.

.

Quoted 35 lines · Junling Zheng

Hi,

Quoted 31 lines · Chao Yu

Hi All,

Quoted 9 lines · Jaegeuk Kim

From: katao <[email protected]>

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?

Quoted 3 lines

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,

Quoted 42 lines

Thanks,
Junling

Quoted 35 lines

Thanks,

Quoted 19 lines

Signed-off-by katao <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 lib/libf2fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

lib/libf2fs.c +2 -1

index 0c684d5..5f11796 100644
@@ -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){
+			/*

lib/libf2fs.c int get_device_info(int i) · line 813

Quoted 10 lines

lib/libf2fs.c +6 -0

+			 * 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) {

.

Quoted 2 lines

.

.

Quoted 40 lines · Chao Yu

Quoted 38 lines · Junling Zheng

Hi,

Quoted 24 lines · Chao Yu

Hi All,

Quoted 9 lines · Jaegeuk Kim

From: katao <[email protected]>

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

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 45 lines

Thanks,

Quoted 42 lines

Thanks,
Junling

Quoted 35 lines

Thanks,

Quoted 19 lines

Signed-off-by katao <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 lib/libf2fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

lib/libf2fs.c +2 -1

index 0c684d5..5f11796 100644
@@ -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){
+			/*

lib/libf2fs.c int get_device_info(int i) · line 813

Quoted 10 lines

lib/libf2fs.c +6 -0

+			 * 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) {

.

Quoted 6 lines

Quoted 2 lines

.

.

Quoted 41 lines · Junling Zheng

Quoted 40 lines · Chao Yu

Quoted 38 lines · Junling Zheng

Hi,

Quoted 24 lines · Chao Yu

Hi All,

Quoted 9 lines · Jaegeuk Kim

From: katao <[email protected]>

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

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

From 9e615dd0a350d336a8067c64d9ef2bb7ce43a3e5 Mon Sep 17 00:00:00 2001

Quoted 41 lines · Junling Zheng

Quoted 40 lines · Chao Yu

Quoted 38 lines · Junling Zheng

Hi,

Quoted 24 lines · Chao Yu

Hi All,

Quoted 9 lines · Jaegeuk Kim

From: katao <[email protected]>

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

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

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,

Quoted 49 lines

Quoted 49 lines

Thanks,

Quoted 44 lines

Thanks,
Junling

Quoted 26 lines

Thanks,

Quoted 24 lines

Signed-off-by katao <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 lib/libf2fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

lib/libf2fs.c +7 -1

index 0c684d5..5f11796 100644
@@ -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;

Quoted 9 lines

lib/libf2fs.c int get_device_info(int i) · line 813

Quoted 5 lines

lib/libf2fs.c +1 -0

+		}
 #endif
 #ifdef BLKGETSIZE64
 		if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {

.

.

Quoted 3 lines

Quoted 2 lines

.

.

Quoted 54 lines · Jaegeuk Kim

Quoted 41 lines · Junling Zheng

Quoted 40 lines · Chao Yu

Quoted 38 lines · Junling Zheng

Hi,

Quoted 24 lines · Chao Yu

Hi All,

Quoted 9 lines · Jaegeuk Kim

From: katao <[email protected]>

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

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

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

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 55 lines · Chao Yu

Quoted 54 lines · Jaegeuk Kim

Quoted 41 lines · Junling Zheng

Quoted 40 lines · Chao Yu

Quoted 38 lines · Junling Zheng

Hi,

Quoted 24 lines · Chao Yu

Hi All,

Quoted 9 lines · Jaegeuk Kim

From: katao <[email protected]>

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

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 5 lines

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.

Quoted 4 lines

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 <[email protected]>
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 <[email protected]>

Signed-off-by Junling Zheng <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>