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 <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.
> 
> 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 <ka...@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 <ka...@xiaomi.com>
Signed-off-by: Junling Zheng <zhengjunl...@huawei.com>
Signed-off-by: Jaegeuk Kim <jaeg...@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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Reply via email to