| Message ID | 1372857920-4678-3-git-send-email-miaox@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Wed, Jul 3, 2013 at 2:25 PM, Miao Xie <miaox@cn.fujitsu.com> wrote: > > +++ b/disk-io.c > @@ -1270,12 +1270,13 @@ static int close_all_devices(struct btrfs_fs_info *fs_info) > while (!list_empty(list)) { > device = list_entry(list->next, struct btrfs_device, dev_list); > list_del_init(&device->dev_list); > - if (device->fd) { > + if (device->fd != -1) { > fsync(device->fd); > if (posix_fadvise(device->fd, 0, 0, POSIX_FADV_DONTNEED)) > fprintf(stderr, "Warning, could not drop caches\n"); > + close(device->fd); > + device->fd = -1; > } > - close(device->fd); > kfree(device->name); > kfree(device->label); > kfree(device); I deal with this part too at https://patchwork.kernel.org/patch/2787291/ Is there any reason to set device->fd to -1 if we just kfree(device) shortly after? thanks -- Filipe David Manana, "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men." -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On wed, 3 Jul 2013 15:17:02 +0100, Filipe David Manana wrote: > On Wed, Jul 3, 2013 at 2:25 PM, Miao Xie <miaox@cn.fujitsu.com> wrote: >> >> +++ b/disk-io.c >> @@ -1270,12 +1270,13 @@ static int close_all_devices(struct btrfs_fs_info *fs_info) >> while (!list_empty(list)) { >> device = list_entry(list->next, struct btrfs_device, dev_list); >> list_del_init(&device->dev_list); >> - if (device->fd) { >> + if (device->fd != -1) { >> fsync(device->fd); >> if (posix_fadvise(device->fd, 0, 0, POSIX_FADV_DONTNEED)) >> fprintf(stderr, "Warning, could not drop caches\n"); >> + close(device->fd); >> + device->fd = -1; >> } >> - close(device->fd); >> kfree(device->name); >> kfree(device->label); >> kfree(device); > > I deal with this part too at https://patchwork.kernel.org/patch/2787291/ Sorry, I don't know you have dealt with it. But your patch didn't fix the problem completely, there are still some functions that you didn't deal with. > Is there any reason to set device->fd to -1 if we just kfree(device) > shortly after? Right, I will update my patch. Thanks Miao > thanks > > > -- > Filipe David Manana, > > "Reasonable men adapt themselves to the world. > Unreasonable men adapt the world to themselves. > That's why all progress depends on unreasonable men." > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 4, 2013 at 2:30 AM, Miao Xie <miaox@cn.fujitsu.com> wrote: > On wed, 3 Jul 2013 15:17:02 +0100, Filipe David Manana wrote: >> On Wed, Jul 3, 2013 at 2:25 PM, Miao Xie <miaox@cn.fujitsu.com> wrote: >>> >>> +++ b/disk-io.c >>> @@ -1270,12 +1270,13 @@ static int close_all_devices(struct btrfs_fs_info *fs_info) >>> while (!list_empty(list)) { >>> device = list_entry(list->next, struct btrfs_device, dev_list); >>> list_del_init(&device->dev_list); >>> - if (device->fd) { >>> + if (device->fd != -1) { >>> fsync(device->fd); >>> if (posix_fadvise(device->fd, 0, 0, POSIX_FADV_DONTNEED)) >>> fprintf(stderr, "Warning, could not drop caches\n"); >>> + close(device->fd); >>> + device->fd = -1; >>> } >>> - close(device->fd); >>> kfree(device->name); >>> kfree(device->label); >>> kfree(device); >> >> I deal with this part too at https://patchwork.kernel.org/patch/2787291/ > > Sorry, I don't know you have dealt with it. But your patch didn't fix the problem completely, > there are still some functions that you didn't deal with. Yes, I was addressing a different problem. My comment was relative only to this change in disk-io.c:close_all_devices(). thanks > >> Is there any reason to set device->fd to -1 if we just kfree(device) >> shortly after? > > Right, I will update my patch. > > Thanks > Miao > >> thanks >> >> >> -- >> Filipe David Manana, >> >> "Reasonable men adapt themselves to the world. >> Unreasonable men adapt the world to themselves. >> That's why all progress depends on unreasonable men." >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- Filipe David Manana, "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men." -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/btrfs-find-root.c b/btrfs-find-root.c index 810d835..3e1396d 100644 --- a/btrfs-find-root.c +++ b/btrfs-find-root.c @@ -76,7 +76,10 @@ static int close_all_devices(struct btrfs_fs_info *fs_info) list = &fs_info->fs_devices->devices; list_for_each(next, list) { device = list_entry(next, struct btrfs_device, dev_list); - close(device->fd); + if (device->fd != -1) { + close(device->fd); + device->fd = -1; + } } return 0; } diff --git a/disk-io.c b/disk-io.c index 9ffe6e4..4003636 100644 --- a/disk-io.c +++ b/disk-io.c @@ -1270,12 +1270,13 @@ static int close_all_devices(struct btrfs_fs_info *fs_info) while (!list_empty(list)) { device = list_entry(list->next, struct btrfs_device, dev_list); list_del_init(&device->dev_list); - if (device->fd) { + if (device->fd != -1) { fsync(device->fd); if (posix_fadvise(device->fd, 0, 0, POSIX_FADV_DONTNEED)) fprintf(stderr, "Warning, could not drop caches\n"); + close(device->fd); + device->fd = -1; } - close(device->fd); kfree(device->name); kfree(device->label); kfree(device); diff --git a/volumes.c b/volumes.c index d6f81f8..b88385b 100644 --- a/volumes.c +++ b/volumes.c @@ -116,6 +116,7 @@ static int device_list_add(const char *path, /* we can safely leave the fs_devices entry around */ return -ENOMEM; } + device->fd = -1; device->devid = devid; memcpy(device->uuid, disk_super->dev_item.uuid, BTRFS_UUID_SIZE); @@ -161,8 +162,10 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices) again: list_for_each(cur, &fs_devices->devices) { device = list_entry(cur, struct btrfs_device, dev_list); - close(device->fd); - device->fd = -1; + if (device->fd != -1) { + close(device->fd); + device->fd = -1; + } device->writeable = 0; }
As we know, the file descriptor 0 is a special number, so we shouldn't use it to initialize the file descriptor of the devices, or we might close this special file descriptor by mistake when we close the devices. "-1" is a better choice. Signed-off-by: Miao Xie <miaox@cn.fujitsu.com> --- btrfs-find-root.c | 5 ++++- disk-io.c | 5 +++-- volumes.c | 7 +++++-- 3 files changed, 12 insertions(+), 5 deletions(-)