Hi Chao, I rethink your comment and more precise error handling for sysfs file handling is valuable. After the patch rework, I will send the V2 patch.
-- Best Regards, Shin'ichiro Kawasaki On 4/19/19 10:18 AM, Shinichiro Kawasaki wrote: > Hi Chao, thanks for your review. > > On 4/17/19 11:21 AM, Chao Yu wrote: >> Hi Shin'ichiro, >> >> On 2019/4/10 16:56, Shin'ichiro Kawasaki wrote: >>> The mkfs.f2fs checks 'zoned' and 'chunk_sectors' sysfs attributes of >>> zoned block devices to place F2FS areas aligned to block zone >>> boundaries. However, mkfs.f2fs code assumes the given device name has >>> corresponding sysfs path under /sys/block directory. This is not the >>> case when partition devices or symbolic links are specified as the >>> command line argument of mkfs.f2fs. In such a case, mkfs.f2fs fails to >>> read sysfs attributes for zoned block devices, handles the device as a >>> regular block device, and do not align F2FS areas to the block zone >>> boundaries. This results in F2FS write failures because of zone write >>> pointer miss-alignment in sequential write required zones. >>> >>> Change mkfs.f2fs to handle sysfs path correctly. Read symbolic link >>> to get canonical sysfs path. If the device is a partition, cut the last >>> directory name in the sysfs path to refer the holder device attributes. >>> >>> Also introduce read_file() and read_sys_attr() helper functions for >>> code simplicity. >>> >>> Signed-off-by: Shin'ichiro Kawasaki <[email protected]> >>> --- >>> lib/libf2fs_zoned.c | 134 ++++++++++++++++++++++++++++++-------------- >>> 1 file changed, 91 insertions(+), 43 deletions(-) >>> >>> diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c >>> index 6e32f32..e645004 100644 >>> --- a/lib/libf2fs_zoned.c >>> +++ b/lib/libf2fs_zoned.c >>> @@ -8,6 +8,7 @@ >>> */ >>> #define _LARGEFILE64_SOURCE >>> >>> +#define _GNU_SOURCE >>> #include <stdio.h> >>> #include <stdlib.h> >>> #include <string.h> >>> @@ -15,6 +16,8 @@ >>> #include <unistd.h> >>> #include <fcntl.h> >>> #include <sys/stat.h> >>> +#include <sys/sysmacros.h> >>> +#include <linux/limits.h> >>> #ifndef ANDROID_WINDOWS_HOST >>> #include <sys/ioctl.h> >>> #endif >>> @@ -24,67 +27,112 @@ >>> >>> #ifdef HAVE_LINUX_BLKZONED_H >>> >>> +/* >>> + * Read up to 255 characters from the first line of a file. Strip the >>> trailing >>> + * newline. >>> + */ >>> +static char *read_file(const char *path) >>> +{ >>> + char line[256], *p = line; >>> + FILE *f; >>> + >>> + f = fopen(path, "rb"); >>> + if (!f) >>> + return NULL; >>> + if (!fgets(line, sizeof(line), f)) >>> + line[0] = '\0'; >>> + strsep(&p, "\n"); >>> + fclose(f); >>> + >>> + return strdup(line); >>> +} >>> + >>> +static char *read_sys_attr(const char *dev_path, const char *attr) >>> +{ >>> + struct stat statbuf; >>> + char *sys_devno_path = NULL; >>> + char sys_path[PATH_MAX]; >>> + ssize_t sz; >>> + char *part_attr_path = NULL; >>> + char *part_str = NULL; >>> + char *delim = NULL; >>> + char *attr_path = NULL; >>> + char *attr_str = NULL; >>> + >>> + if (stat(dev_path, &statbuf) < 0) >>> + goto out; >>> + >>> + if (asprintf(&sys_devno_path, "/sys/dev/block/%d:%d", >>> + major(statbuf.st_rdev), minor(statbuf.st_rdev)) < 0) >>> + goto out; >>> + >>> + sz = readlink(sys_devno_path, sys_path, sizeof(sys_path) - 1); >>> + if (sz < 0) >>> + goto out; >>> + sys_path[sz] = '\0'; >>> + >>> + /* >>> + * If the device is a partition device, cut the device name in the >>> + * canonical sysfs path to obtain the sysfs path of the holder device. >>> + * e.g.: /sys/devices/.../sda/sda1 -> /sys/devices/.../sda >>> + */ >>> + if (asprintf(&part_attr_path, "/sys/dev/block/%s/partition", >>> + sys_path) < 0) >>> + goto out; >>> + part_str = read_file(part_attr_path); >> >> It needs to handle the error of read_file() here? > > I don't think so. The major scenario that read_file() returns NULL is fopen > fails to read /sys/dev/block/%s/partition. It indicates that the target device > is not a partition device. The sys_path and following code is still valid for > the non-partition device. > > The other minor read_file() NULL return scenario is strdup() failure because > of > memory shortage. Do you think f2fs-tools need strdup() return value check? If > you suggest it, I will update the patch. > > Thanks! > >>> + if (part_str && *part_str == '1') { >>> + delim = strrchr(sys_path, '/'); >>> + if (!delim) >>> + goto out; >>> + *delim = '\0'; >>> + } >>> + >>> + if (asprintf(&attr_path, "/sys/dev/block/%s/%s", sys_path, attr) < 0) >>> + goto out; >>> + >>> + attr_str = read_file(attr_path); >>> +out: >>> + free(attr_path); >>> + free(part_str); >>> + free(part_attr_path); >>> + free(sys_devno_path); >>> + return attr_str; >>> +} >>> + >>> void f2fs_get_zoned_model(int i) >>> { >>> struct device_info *dev = c.devices + i; >>> - char str[128]; >>> - FILE *file; >>> - int res; >>> - >>> - /* 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; >>> - >>> - memset(str, 0, sizeof(str)); >>> - res = fscanf(file, "%s", str); >>> - fclose(file); >>> - >>> - if (res != 1) >>> - goto not_zoned; >>> - >>> - if (strcmp(str, "host-aware") == 0) { >>> - dev->zoned_model = F2FS_ZONED_HA; >>> + char *model_str; >>> + >>> + dev->zoned_model = F2FS_ZONED_NONE; >>> + >>> + model_str = read_sys_attr(dev->path, "queue/zoned"); >>> + if (!model_str) >>> return; >>> - } >>> - if (strcmp(str, "host-managed") == 0) { >>> + >>> + if (strcmp(model_str, "host-aware") == 0) >>> + dev->zoned_model = F2FS_ZONED_HA; >>> + else if (strcmp(model_str, "host-managed") == 0) >>> dev->zoned_model = F2FS_ZONED_HM; >>> - return; >>> - } >>> >>> -not_zoned: >>> - dev->zoned_model = F2FS_ZONED_NONE; >>> + free(model_str); >>> } >>> >>> int f2fs_get_zone_blocks(int i) >>> { >>> struct device_info *dev = c.devices + i; >>> uint64_t sectors; >>> - char str[128]; >>> - FILE *file; >>> - int res; >>> + char * cs_str; >>> >>> /* Get zone size */ >>> dev->zone_blocks = 0; >>> >>> - snprintf(str, sizeof(str), >>> - "/sys/block/%s/queue/chunk_sectors", >>> - basename(dev->path)); >>> - file = fopen(str, "r"); >>> - if (!file) >>> - return -1; >>> - >>> - memset(str, 0, sizeof(str)); >>> - res = fscanf(file, "%s", str); >>> - fclose(file); >>> - >>> - if (res != 1) >>> + cs_str = read_sys_attr(dev->path, "queue/chunk_sectors"); >>> + if (!cs_str) >>> return -1; >>> >>> - sectors = atol(str); >>> + sectors = atol(cs_str); >>> + free(cs_str); >>> if (!sectors) >>> return -1; >>> >>> >> > > _______________________________________________ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
