On 2017/09/26 22:08, Qu Wenruo wrote:
>
>
> On 2017年09月26日 13:45, Misono, Tomohiro wrote:
>> Change seen_fsid to hold fd and DIR* in order to keep access to each fs.
>> This will be used for 'subvol delete --commit-after'.
>
> It is already quite good, good enough for the fix.
>
> However just a small point can be further enhanced, commended below.
>
>>
>> Signed-off-by: Tomohiro Misono <misono.tomohiro@xxxxxxxxxxxxxx>
>> ---
>> cmds-filesystem.c | 4 ++--
>> utils.c | 6 +++++-
>> utils.h | 5 ++++-
>> 3 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
>> index c7dae40..4bbff43 100644
>> --- a/cmds-filesystem.c
>> +++ b/cmds-filesystem.c
>> @@ -277,7 +277,7 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices,
>> u64 devs_found = 0;
>> u64 total;
>>
>> - if (add_seen_fsid(fs_devices->fsid, seen_fsid_hash))
>> + if (add_seen_fsid(fs_devices->fsid, seen_fsid_hash, -1, NULL))
>> return;
>>
>> uuid_unparse(fs_devices->fsid, uuidbuf);
>> @@ -324,7 +324,7 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
>> struct btrfs_ioctl_dev_info_args *tmp_dev_info;
>> int ret;
>>
>> - ret = add_seen_fsid(fs_info->fsid, seen_fsid_hash);
>> + ret = add_seen_fsid(fs_info->fsid, seen_fsid_hash, -1, NULL);
>> if (ret == -EEXIST)
>> return 0;
>> else if (ret)
>> diff --git a/utils.c b/utils.c
>> index f91d41e..bdfbfe0 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -1804,7 +1804,8 @@ int is_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[])
>> return 0;
>> }
>>
>> -int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[])
>> +int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[],
>> + int fd, DIR *dirstream)
>> {
>> u8 hash = fsid[0];
>> int slot = hash % SEEN_FSID_HASH_SIZE;
>> @@ -1832,6 +1833,8 @@ insert:
>>
>> alloc->next = NULL;
>> memcpy(alloc->fsid, fsid, BTRFS_FSID_SIZE);
>> + alloc->fd = fd;
>> + alloc->dirstream = dirstream;
>>
>> if (seen)
>> seen->next = alloc;
>> @@ -1851,6 +1854,7 @@ void free_seen_fsid(struct seen_fsid *seen_fsid_hash[])
>> seen = seen_fsid_hash[slot];
>> while (seen) {
>> next = seen->next;
>> + close_file_or_dir(seen->fd, seen->dirstream);
>> free(seen);
>> seen = next;
>> }
>> diff --git a/utils.h b/utils.h
>> index da34e6c..bac7688 100644
>> --- a/utils.h
>> +++ b/utils.h
>> @@ -107,9 +107,12 @@ int get_fsid(const char *path, u8 *fsid, int silent);
>> struct seen_fsid {
>> u8 fsid[BTRFS_FSID_SIZE];
>> struct seen_fsid *next;
>> + int fd;
>
> Will it be possible that the final fd recorded here is invalid or some
> other reason that we failed to execute SYNC ioctl on that fd, but can
> succeeded with other fd?
>
> In that case, a list of fd will help.
>
> Thanks,
> Qu
>
Hello,
I think fd will not be invalidated unless user does because open is
succeeded. Also, if SYNC is failed for one fd, it would fail for other fds
too. So, I think there is no need to keep several fds. What do you think?
By the way, thanks for reviewing whole patches and comments.
I will splits the cleanup for the fourth patch.
Regards,
Tomohiro
>> + DIR *dirstream;
>> };
>> int is_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[]);
>> -int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[]);
>> +int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[],
>> + int fd, DIR *dirstream);
>> void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]);
>>
>> int get_label(const char *btrfs_dev, char *label);
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
