The first 2 patches of this series are not zoned block device specific and fix compilation warnings and modify .gitignore file to handle /tools/f2fs_io/f2fs_io. The last 3 patches improvements of zoned block device support, with the last patch 5 being the most important. Damien Le Moal (5): f2fs-tools: Fix various compilation warnings f2fs-tools: Add f2fs_io to .gitignore f2fs-tools: Improve zoned model check f2fs-tools: Allow using host-aware devices as regular devices f2fs-tools: Fix multi-device format with zoned devices .gitignore | 1 + include/f2fs_fs.h | 8 +++---- lib/libf2fs.c | 53 +++++++++++++++++++++++++++++++-------------- lib/libf2fs_zoned.c | 22 ++++++++++--------- tools/f2fstat.c | 2 +- 5 files changed, 55 insertions(+), 31 deletions(-) -- 2.20.1
Avoid various compilation warnings due to strncpy: libf2fs.c:590:33: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=] snprintf(rootdev, ret, "/dev/%s", buf); ../include/f2fs_fs.h:1384:2: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=] strncpy(buf, features, strlen(features) + 1); f2fstat.c:243:3: warning: ‘strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation] strncpy(ptr_buf, name[i], strlen(name[i])); This patch does not change any functionality. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- include/f2fs_fs.h | 6 +++--- lib/libf2fs.c | 2 +- tools/f2fstat.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h index a730688..e073723 100644 --- a/include/f2fs_fs.h +++ b/include/f2fs_fs.h @@ -1379,9 +1379,9 @@ static inline int parse_feature(struct feature *table, const char *features) { char *buf, *sub, *next; - buf = calloc(strlen(features) + 1, sizeof(char)); - ASSERT(buf); - strncpy(buf, features, strlen(features) + 1); + buf = strdup(features); + if (!buf) + return -1; for (sub = buf; sub && *sub; sub = next ? next + 1 : NULL) { /* Skip the beginning blanks */ diff --git a/lib/libf2fs.c b/lib/libf2fs.c index c692bf2..60b84e0 100644 --- a/lib/libf2fs.c +++ b/lib/libf2fs.c @@ -587,7 +587,7 @@ char *get_rootdev() return NULL; rootdev[ret] = '\0'; - snprintf(rootdev, ret, "/dev/%s", buf); + snprintf(rootdev, ret + 1, "/dev/%s", buf); return rootdev; #endif } diff --git a/tools/f2fstat.c b/tools/f2fstat.c index 7f485b0..0618806 100644 --- a/tools/f2fstat.c +++ b/tools/f2fstat.c @@ -240,7 +240,7 @@ void print_head(char *res) for (i = 0; i < 20; i++) { ptr = (i == 0) ? strtok(res, " ") : strtok(NULL, " "); - strncpy(ptr_buf, name[i], strlen(name[i])); + strcpy(ptr_buf, name[i]); if (i == 1) { prev_index = ptr_buf - buf - 1; } else if (i == 7) { -- 2.20.1 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
GIT ignore compiled executable tools/f2fs_io/f2fs_io. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 3f04e85..c1341da 100644 --- a/.gitignore +++ b/.gitignore @@ -50,6 +50,7 @@ stamp-h1 /tools/fibmap.f2fs /tools/parse.f2fs /tools/f2fscrypt +/tools/f2fs_io/f2fs_io /tools/sg_write_buffer/sg_write_buffer # cscope files -- 2.20.1
Return an error if an unknown zoned model is reported for a device. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- include/f2fs_fs.h | 2 +- lib/libf2fs.c | 8 ++++++-- lib/libf2fs_zoned.c | 20 +++++++++++--------- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h index e073723..97ad774 100644 --- a/include/f2fs_fs.h +++ b/include/f2fs_fs.h @@ -1257,7 +1257,7 @@ blk_zone_cond_str(struct blk_zone *blkz) #endif -extern void f2fs_get_zoned_model(int); +extern int f2fs_get_zoned_model(int); extern int f2fs_get_zone_blocks(int); extern int f2fs_check_zones(int); extern int f2fs_reset_zones(int); diff --git a/lib/libf2fs.c b/lib/libf2fs.c index 60b84e0..5ca1bb0 100644 --- a/lib/libf2fs.c +++ b/lib/libf2fs.c @@ -925,8 +925,12 @@ int get_device_info(int i) } #if !defined(WITH_ANDROID) && defined(__linux__) - if (S_ISBLK(stat_buf->st_mode)) - f2fs_get_zoned_model(i); + if (S_ISBLK(stat_buf->st_mode)) { + if (f2fs_get_zoned_model(i) < 0) { + free(stat_buf); + return -1; + } + } if (dev->zoned_model != F2FS_ZONED_NONE) { if (dev->zoned_model == F2FS_ZONED_HM) diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c index 6e32f32..1a6c5df 100644 --- a/lib/libf2fs_zoned.c +++ b/lib/libf2fs_zoned.c @@ -24,39 +24,41 @@ #ifdef HAVE_LINUX_BLKZONED_H -void f2fs_get_zoned_model(int i) +int f2fs_get_zoned_model(int i) { struct device_info *dev = c.devices + i; char str[128]; FILE *file; int res; + /* Assume regular device */ + dev->zoned_model = F2FS_ZONED_NONE; + /* Check that this is a zoned block device */ snprintf(str, sizeof(str), "/sys/block/%s/queue/zoned", basename(dev->path)); file = fopen(str, "r"); if (!file) - goto not_zoned; + return 0; memset(str, 0, sizeof(str)); res = fscanf(file, "%s", str); fclose(file); if (res != 1) - goto not_zoned; + return 0; if (strcmp(str, "host-aware") == 0) { dev->zoned_model = F2FS_ZONED_HA; - return; - } - if (strcmp(str, "host-managed") == 0) { + } else if (strcmp(str, "host-managed") == 0) { dev->zoned_model = F2FS_ZONED_HM; - return; + } else { + MSG(0, "\tError: Unsupported device zoned model\n"); + return -1; } -not_zoned: - dev->zoned_model = F2FS_ZONED_NONE; + return 0; } int f2fs_get_zone_blocks(int i) -- 2.20.1
Host-aware zoned block devices can accept random writes anywhere and so do not require to be handled under F2FS_ZONED_HM mode. Allow host aware disks to be treated as regular devices if c.zoned_mode is false, that is, if the -m option is not specified in mkfs.f2fs. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- lib/libf2fs.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/lib/libf2fs.c b/lib/libf2fs.c index 5ca1bb0..214c921 100644 --- a/lib/libf2fs.c +++ b/lib/libf2fs.c @@ -933,9 +933,21 @@ int get_device_info(int i) } if (dev->zoned_model != F2FS_ZONED_NONE) { - if (dev->zoned_model == F2FS_ZONED_HM) + if (dev->zoned_model == F2FS_ZONED_HM) { c.zoned_model = F2FS_ZONED_HM; + } else { + /* F2FS_ZONED_HA */ + if (c.zoned_mode) { + c.zoned_model = F2FS_ZONED_HM; + } else { + MSG(0, "Info: treating host-aware zoned block " + "device as regular device\n"); + dev->zoned_model = F2FS_ZONED_NONE; + } + } + } + if (c.zoned_model == F2FS_ZONED_HM) { if (f2fs_get_zone_blocks(i)) { MSG(0, "\tError: Failed to get number of blocks per zone\n"); free(stat_buf); @@ -1071,6 +1083,7 @@ int get_device_info(int i) int f2fs_get_device_info(void) { + bool zoned = false; int i; for (i = 0; i < c.ndevs; i++) @@ -1089,22 +1102,26 @@ int f2fs_get_device_info(void) return -1; } + /* For zoned devices, the zones sizes must be equal */ for (i = 0; i < c.ndevs; i++) { - if (c.devices[i].zoned_model != F2FS_ZONED_NONE) { - if (c.zone_blocks && - c.zone_blocks != c.devices[i].zone_blocks) { - MSG(0, "\tError: not support different zone sizes!!!\n"); - return -1; - } - c.zone_blocks = c.devices[i].zone_blocks; + if (c.devices[i].zoned_model != F2FS_ZONED_HM) + continue; + + zoned = true; + + if (c.zone_blocks && + c.zone_blocks != c.devices[i].zone_blocks) { + MSG(0, "\tError: zones of different size are not supported\n"); + return -1; } + c.zone_blocks = c.devices[i].zone_blocks; } - /* - * Align sections to the device zone size - * and align F2FS zones to the device zones. - */ - if (c.zone_blocks) { + if (zoned) { + /* + * Align sections to the device zone size + * and align F2FS zones to the device zones. + */ c.segs_per_sec = c.zone_blocks / DEFAULT_BLOCKS_PER_SEGMENT; c.secs_per_zone = 1; } else { -- 2.20.1
There is no need to require conventional zones for a zoned block device that is not the first device of a multi-device volume. So exclude the check on the number of conventional zones of the device if the device index is not 0. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- lib/libf2fs_zoned.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c index 1a6c5df..2d8244f 100644 --- a/lib/libf2fs_zoned.c +++ b/lib/libf2fs_zoned.c @@ -204,7 +204,7 @@ int f2fs_check_zones(int j) goto out; } - if (dev->zoned_model == F2FS_ZONED_HM && + if (j == 0 && dev->zoned_model == F2FS_ZONED_HM && !dev->nr_rnd_zones) { ERR_MSG("No conventional zone for super block\n"); ret = -1; -- 2.20.1
On 2019/3/18 14:39, Damien Le Moal wrote: > Avoid various compilation warnings due to strncpy: > > libf2fs.c:590:33: warning: ‘snprintf’ output may be truncated before > the last format character [-Wformat-truncation=] > snprintf(rootdev, ret, "/dev/%s", buf); > > ../include/f2fs_fs.h:1384:2: warning: ‘strncpy’ specified bound > depends on the length of the source argument [-Wstringop-overflow=] > strncpy(buf, features, strlen(features) + 1); > > f2fstat.c:243:3: warning: ‘strncpy’ output truncated before > terminating nul copying as many bytes from a string as its length > [-Wstringop-truncation] > strncpy(ptr_buf, name[i], strlen(name[i])); > > This patch does not change any functionality. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> Reviewed-by: Chao Yu <yuchao0@huawei.com> Thanks, _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2019/3/18 14:39, Damien Le Moal wrote:
> GIT ignore compiled executable tools/f2fs_io/f2fs_io.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Thanks,
On 2019/3/18 14:39, Damien Le Moal wrote:
> Return an error if an unknown zoned model is reported for a device.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Thanks,
On 2019/3/18 14:39, Damien Le Moal wrote: > Host-aware zoned block devices can accept random writes anywhere and so > do not require to be handled under F2FS_ZONED_HM mode. Allow host aware > disks to be treated as regular devices if c.zoned_mode is false, that > is, if the -m option is not specified in mkfs.f2fs. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > lib/libf2fs.c | 43 ++++++++++++++++++++++++++++++------------- > 1 file changed, 30 insertions(+), 13 deletions(-) > > diff --git a/lib/libf2fs.c b/lib/libf2fs.c > index 5ca1bb0..214c921 100644 > --- a/lib/libf2fs.c > +++ b/lib/libf2fs.c > @@ -933,9 +933,21 @@ int get_device_info(int i) > } > > if (dev->zoned_model != F2FS_ZONED_NONE) { > - if (dev->zoned_model == F2FS_ZONED_HM) > + if (dev->zoned_model == F2FS_ZONED_HM) { > c.zoned_model = F2FS_ZONED_HM; > + } else { > + /* F2FS_ZONED_HA */ > + if (c.zoned_mode) { > + c.zoned_model = F2FS_ZONED_HM; May I ask why we change the model from F2FS_ZONED_HA to F2FS_ZONED_HM? Thanks, > + } else { > + MSG(0, "Info: treating host-aware zoned block " > + "device as regular device\n"); > + dev->zoned_model = F2FS_ZONED_NONE; > + } > + } > + } > > + if (c.zoned_model == F2FS_ZONED_HM) { > if (f2fs_get_zone_blocks(i)) { > MSG(0, "\tError: Failed to get number of blocks per zone\n"); > free(stat_buf); > @@ -1071,6 +1083,7 @@ int get_device_info(int i) > > int f2fs_get_device_info(void) > { > + bool zoned = false; > int i; > > for (i = 0; i < c.ndevs; i++) > @@ -1089,22 +1102,26 @@ int f2fs_get_device_info(void) > return -1; > } > > + /* For zoned devices, the zones sizes must be equal */ > for (i = 0; i < c.ndevs; i++) { > - if (c.devices[i].zoned_model != F2FS_ZONED_NONE) { > - if (c.zone_blocks && > - c.zone_blocks != c.devices[i].zone_blocks) { > - MSG(0, "\tError: not support different zone sizes!!!\n"); > - return -1; > - } > - c.zone_blocks = c.devices[i].zone_blocks; > + if (c.devices[i].zoned_model != F2FS_ZONED_HM) > + continue; > + > + zoned = true; > + > + if (c.zone_blocks && > + c.zone_blocks != c.devices[i].zone_blocks) { > + MSG(0, "\tError: zones of different size are not supported\n"); > + return -1; > } > + c.zone_blocks = c.devices[i].zone_blocks; > } > > - /* > - * Align sections to the device zone size > - * and align F2FS zones to the device zones. > - */ > - if (c.zone_blocks) { > + if (zoned) { > + /* > + * Align sections to the device zone size > + * and align F2FS zones to the device zones. > + */ > c.segs_per_sec = c.zone_blocks / DEFAULT_BLOCKS_PER_SEGMENT; > c.secs_per_zone = 1; > } else { >
On 2019/3/18 14:39, Damien Le Moal wrote: > There is no need to require conventional zones for a zoned block device > that is not the first device of a multi-device volume. So exclude the > check on the number of conventional zones of the device if the device > index is not 0. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > lib/libf2fs_zoned.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c > index 1a6c5df..2d8244f 100644 > --- a/lib/libf2fs_zoned.c > +++ b/lib/libf2fs_zoned.c > @@ -204,7 +204,7 @@ int f2fs_check_zones(int j) > goto out; > } > How about adding one line comment here to describe why we handle condition as below for better readability? I guess using your commit message here is enough... :) > - if (dev->zoned_model == F2FS_ZONED_HM && > + if (j == 0 && dev->zoned_model == F2FS_ZONED_HM && > !dev->nr_rnd_zones) { > ERR_MSG("No conventional zone for super block\n"); > ret = -1; >
On 2019/03/21 17:32, Chao Yu wrote: > On 2019/3/18 14:39, Damien Le Moal wrote: >> Host-aware zoned block devices can accept random writes anywhere and so >> do not require to be handled under F2FS_ZONED_HM mode. Allow host aware >> disks to be treated as regular devices if c.zoned_mode is false, that >> is, if the -m option is not specified in mkfs.f2fs. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> lib/libf2fs.c | 43 ++++++++++++++++++++++++++++++------------- >> 1 file changed, 30 insertions(+), 13 deletions(-) >> >> diff --git a/lib/libf2fs.c b/lib/libf2fs.c >> index 5ca1bb0..214c921 100644 >> --- a/lib/libf2fs.c >> +++ b/lib/libf2fs.c >> @@ -933,9 +933,21 @@ int get_device_info(int i) >> } >> >> if (dev->zoned_model != F2FS_ZONED_NONE) { >> - if (dev->zoned_model == F2FS_ZONED_HM) >> + if (dev->zoned_model == F2FS_ZONED_HM) { >> c.zoned_model = F2FS_ZONED_HM; >> + } else { >> + /* F2FS_ZONED_HA */ >> + if (c.zoned_mode) { >> + c.zoned_model = F2FS_ZONED_HM; > > May I ask why we change the model from F2FS_ZONED_HA to F2FS_ZONED_HM? > > Thanks, There is some confusion I think in the code between disk zoned model and f2fs zoned mode. Currently, F2FS_ZONED_HA is not tested anywhere to drive the check against c.zoned_mode. Before this patch, the code was: if (dev->zoned_model != F2FS_ZONED_NONE) { if (dev->zoned_model == F2FS_ZONED_HM) c.zoned_model = F2FS_ZONED_HM; So only HM disks end up setting "c.zoned_model = F2FS_ZONED_HM" which requires c.zoned_mode to be set. For an F2FS_ZONED_HA disk, c.zoned_model stays as NONE and c.zone_blocks is set, regardless of c.zoned_mode setting. The end result is that the HA disk will be used as a regular disk when -m is not specified on mkfs command line, and as a zoned disk when -m is specified. However, many areas of the code check for c.zoned_model == F2FS_ZONED_HM, and that is always false for HA disks. The idea here is to allow a cleaner control over HA disk use: either use them as regular disks and ignore that they are zoned, or use them in the same manner as HM disks, ignoring the fact that HA disks can accept random writes. Anything in between these 2 modes does not really make sense. Now, rereading the code and explaining all this, I realize that the commit message is actually not correct at all and misleading to the intent of the patch. I will revisit it and check again this patch. We could actually drop it for now and I can resend later. In any case, I am sending a v2 for this series. Thanks for your review ! > >> + } else { >> + MSG(0, "Info: treating host-aware zoned block " >> + "device as regular device\n"); >> + dev->zoned_model = F2FS_ZONED_NONE; >> + } >> + } >> + } >> >> + if (c.zoned_model == F2FS_ZONED_HM) { >> if (f2fs_get_zone_blocks(i)) { >> MSG(0, "\tError: Failed to get number of blocks per zone\n"); >> free(stat_buf); >> @@ -1071,6 +1083,7 @@ int get_device_info(int i) >> >> int f2fs_get_device_info(void) >> { >> + bool zoned = false; >> int i; >> >> for (i = 0; i < c.ndevs; i++) >> @@ -1089,22 +1102,26 @@ int f2fs_get_device_info(void) >> return -1; >> } >> >> + /* For zoned devices, the zones sizes must be equal */ >> for (i = 0; i < c.ndevs; i++) { >> - if (c.devices[i].zoned_model != F2FS_ZONED_NONE) { >> - if (c.zone_blocks && >> - c.zone_blocks != c.devices[i].zone_blocks) { >> - MSG(0, "\tError: not support different zone sizes!!!\n"); >> - return -1; >> - } >> - c.zone_blocks = c.devices[i].zone_blocks; >> + if (c.devices[i].zoned_model != F2FS_ZONED_HM) >> + continue; >> + >> + zoned = true; >> + >> + if (c.zone_blocks && >> + c.zone_blocks != c.devices[i].zone_blocks) { >> + MSG(0, "\tError: zones of different size are not supported\n"); >> + return -1; >> } >> + c.zone_blocks = c.devices[i].zone_blocks; >> } >> >> - /* >> - * Align sections to the device zone size >> - * and align F2FS zones to the device zones. >> - */ >> - if (c.zone_blocks) { >> + if (zoned) { >> + /* >> + * Align sections to the device zone size >> + * and align F2FS zones to the device zones. >> + */ >> c.segs_per_sec = c.zone_blocks / DEFAULT_BLOCKS_PER_SEGMENT; >> c.secs_per_zone = 1; >> } else { >> > -- Damien Le Moal Western Digital Research
On 2019/03/21 17:41, Chao Yu wrote: > On 2019/3/18 14:39, Damien Le Moal wrote: >> There is no need to require conventional zones for a zoned block device >> that is not the first device of a multi-device volume. So exclude the >> check on the number of conventional zones of the device if the device >> index is not 0. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> lib/libf2fs_zoned.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c >> index 1a6c5df..2d8244f 100644 >> --- a/lib/libf2fs_zoned.c >> +++ b/lib/libf2fs_zoned.c >> @@ -204,7 +204,7 @@ int f2fs_check_zones(int j) >> goto out; >> } >> > > How about adding one line comment here to describe why we handle condition > as below for better readability? I guess using your commit message here is > enough... :) Indeed, that would be better. Will add that. Thanks ! > >> - if (dev->zoned_model == F2FS_ZONED_HM && >> + if (j == 0 && dev->zoned_model == F2FS_ZONED_HM && >> !dev->nr_rnd_zones) { >> ERR_MSG("No conventional zone for super block\n"); >> ret = -1; >> > -- Damien Le Moal Western Digital Research
On 2019/3/21 17:29, Damien Le Moal wrote: > On 2019/03/21 17:32, Chao Yu wrote: >> On 2019/3/18 14:39, Damien Le Moal wrote: >>> Host-aware zoned block devices can accept random writes anywhere and so >>> do not require to be handled under F2FS_ZONED_HM mode. Allow host aware >>> disks to be treated as regular devices if c.zoned_mode is false, that >>> is, if the -m option is not specified in mkfs.f2fs. >>> >>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>> --- >>> lib/libf2fs.c | 43 ++++++++++++++++++++++++++++++------------- >>> 1 file changed, 30 insertions(+), 13 deletions(-) >>> >>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c >>> index 5ca1bb0..214c921 100644 >>> --- a/lib/libf2fs.c >>> +++ b/lib/libf2fs.c >>> @@ -933,9 +933,21 @@ int get_device_info(int i) >>> } >>> >>> if (dev->zoned_model != F2FS_ZONED_NONE) { >>> - if (dev->zoned_model == F2FS_ZONED_HM) >>> + if (dev->zoned_model == F2FS_ZONED_HM) { >>> c.zoned_model = F2FS_ZONED_HM; >>> + } else { >>> + /* F2FS_ZONED_HA */ >>> + if (c.zoned_mode) { >>> + c.zoned_model = F2FS_ZONED_HM; >> >> May I ask why we change the model from F2FS_ZONED_HA to F2FS_ZONED_HM? >> >> Thanks, > > There is some confusion I think in the code between disk zoned model and f2fs > zoned mode. Currently, F2FS_ZONED_HA is not tested anywhere to drive the check > against c.zoned_mode. Before this patch, the code was: > > if (dev->zoned_model != F2FS_ZONED_NONE) { > if (dev->zoned_model == F2FS_ZONED_HM) > c.zoned_model = F2FS_ZONED_HM; > > So only HM disks end up setting "c.zoned_model = F2FS_ZONED_HM" which requires > c.zoned_mode to be set. For an F2FS_ZONED_HA disk, c.zoned_model stays as NONE > and c.zone_blocks is set, regardless of c.zoned_mode setting. The end result is > that the HA disk will be used as a regular disk when -m is not specified on mkfs > command line, and as a zoned disk when -m is specified. However, many areas of > the code check for c.zoned_model == F2FS_ZONED_HM, and that is always false for > HA disks. > > The idea here is to allow a cleaner control over HA disk use: either use them as > regular disks and ignore that they are zoned, or use them in the same manner as > HM disks, ignoring the fact that HA disks can accept random writes. Anything in > between these 2 modes does not really make sense. Thanks for your explanation. It looks like with current implementation that we treat HA disk as HM one, is that mean we consider more about performance? since on HM device we only allow sequential IO, which can make better performance on initial disk. > > Now, rereading the code and explaining all this, I realize that the commit > message is actually not correct at all and misleading to the intent of the > patch. I will revisit it and check again this patch. We could actually drop it > for now and I can resend later. > > In any case, I am sending a v2 for this series. Thanks, will wait for that. :) Thanks, > > Thanks for your review ! > > >> >>> + } else { >>> + MSG(0, "Info: treating host-aware zoned block " >>> + "device as regular device\n"); >>> + dev->zoned_model = F2FS_ZONED_NONE; >>> + } >>> + } >>> + } >>> >>> + if (c.zoned_model == F2FS_ZONED_HM) { >>> if (f2fs_get_zone_blocks(i)) { >>> MSG(0, "\tError: Failed to get number of blocks per zone\n"); >>> free(stat_buf); >>> @@ -1071,6 +1083,7 @@ int get_device_info(int i) >>> >>> int f2fs_get_device_info(void) >>> { >>> + bool zoned = false; >>> int i; >>> >>> for (i = 0; i < c.ndevs; i++) >>> @@ -1089,22 +1102,26 @@ int f2fs_get_device_info(void) >>> return -1; >>> } >>> >>> + /* For zoned devices, the zones sizes must be equal */ >>> for (i = 0; i < c.ndevs; i++) { >>> - if (c.devices[i].zoned_model != F2FS_ZONED_NONE) { >>> - if (c.zone_blocks && >>> - c.zone_blocks != c.devices[i].zone_blocks) { >>> - MSG(0, "\tError: not support different zone sizes!!!\n"); >>> - return -1; >>> - } >>> - c.zone_blocks = c.devices[i].zone_blocks; >>> + if (c.devices[i].zoned_model != F2FS_ZONED_HM) >>> + continue; >>> + >>> + zoned = true; >>> + >>> + if (c.zone_blocks && >>> + c.zone_blocks != c.devices[i].zone_blocks) { >>> + MSG(0, "\tError: zones of different size are not supported\n"); >>> + return -1; >>> } >>> + c.zone_blocks = c.devices[i].zone_blocks; >>> } >>> >>> - /* >>> - * Align sections to the device zone size >>> - * and align F2FS zones to the device zones. >>> - */ >>> - if (c.zone_blocks) { >>> + if (zoned) { >>> + /* >>> + * Align sections to the device zone size >>> + * and align F2FS zones to the device zones. >>> + */ >>> c.segs_per_sec = c.zone_blocks / DEFAULT_BLOCKS_PER_SEGMENT; >>> c.secs_per_zone = 1; >>> } else { >>> >> > >
On 03/18, Damien Le Moal wrote: > Return an error if an unknown zoned model is reported for a device. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > include/f2fs_fs.h | 2 +- > lib/libf2fs.c | 8 ++++++-- > lib/libf2fs_zoned.c | 20 +++++++++++--------- > 3 files changed, 18 insertions(+), 12 deletions(-) > > diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h > index e073723..97ad774 100644 > --- a/include/f2fs_fs.h > +++ b/include/f2fs_fs.h > @@ -1257,7 +1257,7 @@ blk_zone_cond_str(struct blk_zone *blkz) > > #endif > > -extern void f2fs_get_zoned_model(int); > +extern int f2fs_get_zoned_model(int); > extern int f2fs_get_zone_blocks(int); > extern int f2fs_check_zones(int); > extern int f2fs_reset_zones(int); > diff --git a/lib/libf2fs.c b/lib/libf2fs.c > index 60b84e0..5ca1bb0 100644 > --- a/lib/libf2fs.c > +++ b/lib/libf2fs.c > @@ -925,8 +925,12 @@ int get_device_info(int i) > } > > #if !defined(WITH_ANDROID) && defined(__linux__) > - if (S_ISBLK(stat_buf->st_mode)) > - f2fs_get_zoned_model(i); > + if (S_ISBLK(stat_buf->st_mode)) { > + if (f2fs_get_zoned_model(i) < 0) { > + free(stat_buf); > + return -1; > + } > + } > > if (dev->zoned_model != F2FS_ZONED_NONE) { > if (dev->zoned_model == F2FS_ZONED_HM) > diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c > index 6e32f32..1a6c5df 100644 > --- a/lib/libf2fs_zoned.c > +++ b/lib/libf2fs_zoned.c > @@ -24,39 +24,41 @@ > > #ifdef HAVE_LINUX_BLKZONED_H > > -void f2fs_get_zoned_model(int i) > +int f2fs_get_zoned_model(int i) > { > struct device_info *dev = c.devices + i; > char str[128]; > FILE *file; > int res; > > + /* Assume regular device */ > + dev->zoned_model = F2FS_ZONED_NONE; > + > /* Check that this is a zoned block device */ > snprintf(str, sizeof(str), > "/sys/block/%s/queue/zoned", > basename(dev->path)); > file = fopen(str, "r"); > if (!file) > - goto not_zoned; > + return 0; > > memset(str, 0, sizeof(str)); > res = fscanf(file, "%s", str); > fclose(file); > > if (res != 1) > - goto not_zoned; > + return 0; > > if (strcmp(str, "host-aware") == 0) { > dev->zoned_model = F2FS_ZONED_HA; > - return; > - } > - if (strcmp(str, "host-managed") == 0) { > + } else if (strcmp(str, "host-managed") == 0) { > dev->zoned_model = F2FS_ZONED_HM; > - return; > + } else { If "sys/block/%s/queue/zoned" reports "none", we can't format the device. :) > + MSG(0, "\tError: Unsupported device zoned model\n"); > + return -1; > } > > -not_zoned: > - dev->zoned_model = F2FS_ZONED_NONE; > + return 0; > } > > int f2fs_get_zone_blocks(int i) > -- > 2.20.1
On 2019/03/27 3:24, Jaegeuk Kim wrote: > On 03/18, Damien Le Moal wrote: >> Return an error if an unknown zoned model is reported for a device. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> include/f2fs_fs.h | 2 +- >> lib/libf2fs.c | 8 ++++++-- >> lib/libf2fs_zoned.c | 20 +++++++++++--------- >> 3 files changed, 18 insertions(+), 12 deletions(-) >> >> diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h >> index e073723..97ad774 100644 >> --- a/include/f2fs_fs.h >> +++ b/include/f2fs_fs.h >> @@ -1257,7 +1257,7 @@ blk_zone_cond_str(struct blk_zone *blkz) >> >> #endif >> >> -extern void f2fs_get_zoned_model(int); >> +extern int f2fs_get_zoned_model(int); >> extern int f2fs_get_zone_blocks(int); >> extern int f2fs_check_zones(int); >> extern int f2fs_reset_zones(int); >> diff --git a/lib/libf2fs.c b/lib/libf2fs.c >> index 60b84e0..5ca1bb0 100644 >> --- a/lib/libf2fs.c >> +++ b/lib/libf2fs.c >> @@ -925,8 +925,12 @@ int get_device_info(int i) >> } >> >> #if !defined(WITH_ANDROID) && defined(__linux__) >> - if (S_ISBLK(stat_buf->st_mode)) >> - f2fs_get_zoned_model(i); >> + if (S_ISBLK(stat_buf->st_mode)) { >> + if (f2fs_get_zoned_model(i) < 0) { >> + free(stat_buf); >> + return -1; >> + } >> + } >> >> if (dev->zoned_model != F2FS_ZONED_NONE) { >> if (dev->zoned_model == F2FS_ZONED_HM) >> diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c >> index 6e32f32..1a6c5df 100644 >> --- a/lib/libf2fs_zoned.c >> +++ b/lib/libf2fs_zoned.c >> @@ -24,39 +24,41 @@ >> >> #ifdef HAVE_LINUX_BLKZONED_H >> >> -void f2fs_get_zoned_model(int i) >> +int f2fs_get_zoned_model(int i) >> { >> struct device_info *dev = c.devices + i; >> char str[128]; >> FILE *file; >> int res; >> >> + /* Assume regular device */ >> + dev->zoned_model = F2FS_ZONED_NONE; >> + >> /* Check that this is a zoned block device */ >> snprintf(str, sizeof(str), >> "/sys/block/%s/queue/zoned", >> basename(dev->path)); >> file = fopen(str, "r"); >> if (!file) >> - goto not_zoned; >> + return 0; >> >> memset(str, 0, sizeof(str)); >> res = fscanf(file, "%s", str); >> fclose(file); >> >> if (res != 1) >> - goto not_zoned; >> + return 0; >> >> if (strcmp(str, "host-aware") == 0) { >> dev->zoned_model = F2FS_ZONED_HA; >> - return; >> - } >> - if (strcmp(str, "host-managed") == 0) { >> + } else if (strcmp(str, "host-managed") == 0) { >> dev->zoned_model = F2FS_ZONED_HM; >> - return; >> + } else { > > If "sys/block/%s/queue/zoned" reports "none", we can't format the device. :) Arg ! Yes ! Will fix that. > >> + MSG(0, "\tError: Unsupported device zoned model\n"); >> + return -1; >> } >> >> -not_zoned: >> - dev->zoned_model = F2FS_ZONED_NONE; >> + return 0; >> } >> >> int f2fs_get_zone_blocks(int i) >> -- >> 2.20.1 > -- Damien Le Moal Western Digital Research