btrfs-progs: suggestion of removing --commit-after option of subvol delete

btrfs-progs: suggestion of removing --commit-after option of subvol delete

[Misono, Tomohiro]

Hello,

I read the code of "subvolume delete" and found that --commit-after option is
not working well.

Since it issues BTRFS_IOC_START/WAIT_SYNC to the last fd (of directory
containing the last deleted subvolume),
1. sync operation affects only the last fd's filesystem.
   ("subvolume delete" can take multiple subvolumes on different filesystems.)
2. if the last delete action fails to open the path (fd == -1),
   SYNC is not issued at all.

One solution is to keep every fd for deleted subvolumes, but I think it takes
too much cost. Since we can just use "btrfs filesystem sync" after delete if
needed, I think it is ok to remove --comit-after option.

Regards,
Tomohiro Misono
(misono.tomohiro@jp.fujitsu.com)

Re: btrfs-progs: suggestion of removing --commit-after option of subvol delete

[Qu Wenruo]

On 2017年09月19日 15:50, Misono, Tomohiro wrote:
> Hello,
> 
> I read the code of "subvolume delete" and found that --commit-after option is
> not working well.
> 
> Since it issues BTRFS_IOC_START/WAIT_SYNC to the last fd (of directory
> containing the last deleted subvolume),
> 1. sync operation affects only the last fd's filesystem.
>     ("subvolume delete" can take multiple subvolumes on different filesystems.)
> 2. if the last delete action fails to open the path (fd == -1),
>     SYNC is not issued at all.
> 
> One solution is to keep every fd for deleted subvolumes, but I think it takes
> too much cost. Since we can just use "btrfs filesystem sync" after delete if
> needed, I think it is ok to remove --comit-after option.

Personally speaking I'm OK removing --commit-after, as implementing a 
full working --commit-after seems too complex for a minor feature.
(Need to finding the same fs of multiple subvolume and doing commit for 
each fs, and fallback to other fd if open failed)

Since --commit-after is a relatively lightweight solution compared to 
--commit-each, and both can only ensure subvolume doesn't show up, while 
"fi sync" can do a "deeper" sync to ensure the whole subvolume get 
removed on disk.

But instead of deleting the option, it would be better to keep it 
deprecated for a while.
Showing a message informing user this option is deprecated and falling 
back to --commit-each seems to be a better solution.

Thanks,
Qu
> 
> Regards,
> Tomohiro Misono
> (misono.tomohiro@jp.fujitsu.com)
> 
> --
> 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
> 

Re: btrfs-progs: suggestion of removing --commit-after option of subvol delete

[David Sterba]

On Tue, Sep 19, 2017 at 04:50:04PM +0900, Misono, Tomohiro wrote:
> I read the code of "subvolume delete" and found that --commit-after option is
> not working well.
> 
> Since it issues BTRFS_IOC_START/WAIT_SYNC to the last fd (of directory
> containing the last deleted subvolume),
> 1. sync operation affects only the last fd's filesystem.
>    ("subvolume delete" can take multiple subvolumes on different filesystems.)

You're right, though I don't think this is a common usecase.

> 2. if the last delete action fails to open the path (fd == -1),
>    SYNC is not issued at all.
> 
> One solution is to keep every fd for deleted subvolumes, but I think it takes
> too much cost.

How do you evaluate the cost? We'd have to keep track of all the
distinct filesystems of the subvolumes, so we issue sync on each of them
in the end.

> Since we can just use "btrfs filesystem sync" after delete if
> needed, I think it is ok to remove --comit-after option.

The point of the option is to allow the sync in the same command as the
subvolume deletion. Even with the sync, the user would still have to
know all the filesystems where the the subvolumes were, so some way of
tracking them would need to be done.

I don't want to remove the option unless it's really not working as
expected and could mislead the users. What you found are bugs that can
be fixed.

Re: btrfs-progs: suggestion of removing --commit-after option of subvol delete

[Misono, Tomohiro]

On 2017/09/19 23:48, David Sterba wrote:
> On Tue, Sep 19, 2017 at 04:50:04PM +0900, Misono, Tomohiro wrote:
>> I read the code of "subvolume delete" and found that --commit-after option is
>> not working well.
>>
>> Since it issues BTRFS_IOC_START/WAIT_SYNC to the last fd (of directory
>> containing the last deleted subvolume),
>> 1. sync operation affects only the last fd's filesystem.
>>    ("subvolume delete" can take multiple subvolumes on different filesystems.)
> 
> You're right, though I don't think this is a common usecase.
> 
>> 2. if the last delete action fails to open the path (fd == -1),
>>    SYNC is not issued at all.
>>
>> One solution is to keep every fd for deleted subvolumes, but I think it takes
>> too much cost.
> 
> How do you evaluate the cost? We'd have to keep track of all the
> distinct filesystems of the subvolumes, so we issue sync on each of them
> in the end.

This potentially keep many fds long time. Since the number of open file descriptors
is limited, I don't think it is good to keep fds long time.

>> Since we can just use "btrfs filesystem sync" after delete if
>> needed, I think it is ok to remove --comit-after option.
> 
> The point of the option is to allow the sync in the same command as the
> subvolume deletion. Even with the sync, the user would still have to
> know all the filesystems where the the subvolumes were, so some way of
> tracking them would need to be done.
> 
> I don't want to remove the option unless it's really not working as
> expected and could mislead the users. What you found are bugs that can
> be fixed.
> 
> 

Re: btrfs-progs: suggestion of removing --commit-after option of subvol delete

[Qu Wenruo]

On 2017年09月19日 22:48, David Sterba wrote:
> On Tue, Sep 19, 2017 at 04:50:04PM +0900, Misono, Tomohiro wrote:
>> I read the code of "subvolume delete" and found that --commit-after option is
>> not working well.
>>
>> Since it issues BTRFS_IOC_START/WAIT_SYNC to the last fd (of directory
>> containing the last deleted subvolume),
>> 1. sync operation affects only the last fd's filesystem.
>>     ("subvolume delete" can take multiple subvolumes on different filesystems.)
> 
> You're right, though I don't think this is a common usecase.
> 
>> 2. if the last delete action fails to open the path (fd == -1),
>>     SYNC is not issued at all.
>>
>> One solution is to keep every fd for deleted subvolumes, but I think it takes
>> too much cost.
> 
> How do you evaluate the cost? We'd have to keep track of all the
> distinct filesystems of the subvolumes, so we issue sync on each of them
> in the end.

The costly part will be tracking the filesystems of subvolumes.
We must do it for each subvolume and batch them to address the final 
transaction commit for each fs.

I didn't see any easy ioctl to get the UUID from fd, meaning (if didn't 
miss anything) we need to iterate the path until reaching the mount 
boundary and then refer to mountinfo to find the fs.

Not to mention that the possible softlink in the path may make things 
more complex.

Yes, this may be fixed with tons of code, but I don't think the 
complexity worthy for a minor feature.
(Remember how --rootdir get untested and buggy over time? And we even 
don't know how to make test case to verify --commit-after/each)

Thanks,
Qu

> 
>> Since we can just use "btrfs filesystem sync" after delete if
>> needed, I think it is ok to remove --comit-after option.
> 
> The point of the option is to allow the sync in the same command as the
> subvolume deletion. Even with the sync, the user would still have to
> know all the filesystems where the the subvolumes were, so some way of
> tracking them would need to be done.
> 
> I don't want to remove the option unless it's really not working as
> expected and could mislead the users. What you found are bugs that can
> be fixed.
> --
> 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
> 

Re: btrfs-progs: suggestion of removing --commit-after option of subvol delete

[David Sterba]

On Wed, Sep 20, 2017 at 08:22:54AM +0800, Qu Wenruo wrote:
> The costly part will be tracking the filesystems of subvolumes.
> We must do it for each subvolume and batch them to address the final 
> transaction commit for each fs.
> 
> I didn't see any easy ioctl to get the UUID from fd, meaning (if didn't 
> miss anything) we need to iterate the path until reaching the mount 
> boundary and then refer to mountinfo to find the fs.

The FS_INFO ioctl will tell you the fsid from a fd.  Iterating the paths
will not work reliably, due to some potentially very creative use of
mounts that could build up the path components.

> Not to mention that the possible softlink in the path may make things 
> more complex.
> 
> Yes, this may be fixed with tons of code, but I don't think the 
> complexity worthy for a minor feature.

So what if we fix it like that:

- iterate over subvolumes
- check if the fsid is same as for the previous subvolume
  - if it is, continue
  - if not, do the sync

IOW, sync when we leave subvolumes of one filesystem. In the degenerate
case, we can have subvolumes interleaved that we'd sync after each,
effectively --commit-each.

The simple tracking should avoid keeping all the filedescriptors open
and I think this won't need tons of code to implement.

The typical could be 'btrfs subvol delete -c path/*', ie. deleting from
just one filesystem. If we happen to cross more filesystems, each of
them will get the sync, but possibly more than one. I don't think this
is too serious.

Re: btrfs-progs: suggestion of removing --commit-after option of subvol delete

[Qu Wenruo]

On 2017年09月20日 22:03, David Sterba wrote:
> On Wed, Sep 20, 2017 at 08:22:54AM +0800, Qu Wenruo wrote:
>> The costly part will be tracking the filesystems of subvolumes.
>> We must do it for each subvolume and batch them to address the final
>> transaction commit for each fs.
>>
>> I didn't see any easy ioctl to get the UUID from fd, meaning (if didn't
>> miss anything) we need to iterate the path until reaching the mount
>> boundary and then refer to mountinfo to find the fs.
> 
> The FS_INFO ioctl will tell you the fsid from a fd.  Iterating the paths
> will not work reliably, due to some potentially very creative use of
> mounts that could build up the path components.

The biggest concern is about how to get fsid reliably.
And since FS_INFO ioctl solves this, I'm completely fine.
(I just search "UUID" for ioctls and get no hit, so I assume it may be a 
problem)

> 
>> Not to mention that the possible softlink in the path may make things
>> more complex.
>>
>> Yes, this may be fixed with tons of code, but I don't think the
>> complexity worthy for a minor feature.
> 
> So what if we fix it like that:
> 
> - iterate over subvolumes
> - check if the fsid is same as for the previous subvolume
>    - if it is, continue
>    - if not, do the sync

I prefer to maintain a subvolume <-> fs mapping, and only address 
transaction commit after all subvolumes are iterated.

> 
> IOW, sync when we leave subvolumes of one filesystem. In the degenerate
> case, we can have subvolumes interleaved that we'd sync after each,
> effectively --commit-each.

Since we have a subvolume <-> fs mapping, we can commit each fs after 
iterating all subvolumes.

AFAIK "--commit-after" user would like to introduce the minimal commits, 
so each fs only get committed once seems better to me.

> 
> The simple tracking should avoid keeping all the filedescriptors open
> and I think this won't need tons of code to implement.

Indeed, since there is reliable fs UUID ioctl.

And above subvolume <-> fs mapping can also be used to resolve missing 
fd problems.
Just adding a fds <-> fs mapping, and try to call commit ioctl on first 
good fd.

> 
> The typical could be 'btrfs subvol delete -c path/*', ie. deleting from
> just one filesystem. If we happen to cross more filesystems, each of
> them will get the sync, but possibly more than one. I don't think this
> is too serious.

Well, after we implemented above user case, some facility can be reused 
for this case.

Thanks,
Qu
> --
> 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
> 

Re: btrfs-progs: suggestion of removing --commit-after option of subvol delete

[Misono, Tomohiro]

On 2017/09/20 23:21, Qu Wenruo wrote:
> 
> 
> On 2017年09月20日 22:03, David Sterba wrote:
>> On Wed, Sep 20, 2017 at 08:22:54AM +0800, Qu Wenruo wrote:
>>> The costly part will be tracking the filesystems of subvolumes.
>>> We must do it for each subvolume and batch them to address the final
>>> transaction commit for each fs.
>>>
>>> I didn't see any easy ioctl to get the UUID from fd, meaning (if didn't
>>> miss anything) we need to iterate the path until reaching the mount
>>> boundary and then refer to mountinfo to find the fs.
>>
>> The FS_INFO ioctl will tell you the fsid from a fd.  Iterating the paths
>> will not work reliably, due to some potentially very creative use of
>> mounts that could build up the path components.
> 
> The biggest concern is about how to get fsid reliably.
> And since FS_INFO ioctl solves this, I'm completely fine.
> (I just search "UUID" for ioctls and get no hit, so I assume it may be a 
> problem)
> 
>>
>>> Not to mention that the possible softlink in the path may make things
>>> more complex.
>>>
>>> Yes, this may be fixed with tons of code, but I don't think the
>>> complexity worthy for a minor feature.
>>
>> So what if we fix it like that:
>>
>> - iterate over subvolumes
>> - check if the fsid is same as for the previous subvolume
>>    - if it is, continue
>>    - if not, do the sync
> 
> I prefer to maintain a subvolume <-> fs mapping, and only address 
> transaction commit after all subvolumes are iterated.
> 
>>
>> IOW, sync when we leave subvolumes of one filesystem. In the degenerate
>> case, we can have subvolumes interleaved that we'd sync after each,
>> effectively --commit-each.
> 
> Since we have a subvolume <-> fs mapping, we can commit each fs after 
> iterating all subvolumes.
> 
> AFAIK "--commit-after" user would like to introduce the minimal commits, 
> so each fs only get committed once seems better to me.
> 
>>
>> The simple tracking should avoid keeping all the filedescriptors open
>> and I think this won't need tons of code to implement.
> 
> Indeed, since there is reliable fs UUID ioctl.
> 
> And above subvolume <-> fs mapping can also be used to resolve missing 
> fd problems.
> Just adding a fds <-> fs mapping, and try to call commit ioctl on first 
> good fd.
> 
>>
>> The typical could be 'btrfs subvol delete -c path/*', ie. deleting from
>> just one filesystem. If we happen to cross more filesystems, each of
>> them will get the sync, but possibly more than one. I don't think this
>> is too serious.
> 
> Well, after we implemented above user case, some facility can be reused 
> for this case.
> 
> Thanks,
> Qu

OK, I understood the points and will try to make a patch.
btw, by saying "some facility can be reused", do you mean we can reuse
fds <-> fs mapping function to other than delete?

Thanks,
Tomohiro

Re: btrfs-progs: suggestion of removing --commit-after option of subvol delete

[Qu Wenruo]

On 2017年09月21日 10:49, Misono, Tomohiro wrote:
> On 2017/09/20 23:21, Qu Wenruo wrote:
>>
>>
>> On 2017年09月20日 22:03, David Sterba wrote:
>>> On Wed, Sep 20, 2017 at 08:22:54AM +0800, Qu Wenruo wrote:
>>>> The costly part will be tracking the filesystems of subvolumes.
>>>> We must do it for each subvolume and batch them to address the final
>>>> transaction commit for each fs.
>>>>
>>>> I didn't see any easy ioctl to get the UUID from fd, meaning (if didn't
>>>> miss anything) we need to iterate the path until reaching the mount
>>>> boundary and then refer to mountinfo to find the fs.
>>>
>>> The FS_INFO ioctl will tell you the fsid from a fd.  Iterating the paths
>>> will not work reliably, due to some potentially very creative use of
>>> mounts that could build up the path components.
>>
>> The biggest concern is about how to get fsid reliably.
>> And since FS_INFO ioctl solves this, I'm completely fine.
>> (I just search "UUID" for ioctls and get no hit, so I assume it may be a
>> problem)
>>
>>>
>>>> Not to mention that the possible softlink in the path may make things
>>>> more complex.
>>>>
>>>> Yes, this may be fixed with tons of code, but I don't think the
>>>> complexity worthy for a minor feature.
>>>
>>> So what if we fix it like that:
>>>
>>> - iterate over subvolumes
>>> - check if the fsid is same as for the previous subvolume
>>>     - if it is, continue
>>>     - if not, do the sync
>>
>> I prefer to maintain a subvolume <-> fs mapping, and only address
>> transaction commit after all subvolumes are iterated.
>>
>>>
>>> IOW, sync when we leave subvolumes of one filesystem. In the degenerate
>>> case, we can have subvolumes interleaved that we'd sync after each,
>>> effectively --commit-each.
>>
>> Since we have a subvolume <-> fs mapping, we can commit each fs after
>> iterating all subvolumes.
>>
>> AFAIK "--commit-after" user would like to introduce the minimal commits,
>> so each fs only get committed once seems better to me.
>>
>>>
>>> The simple tracking should avoid keeping all the filedescriptors open
>>> and I think this won't need tons of code to implement.
>>
>> Indeed, since there is reliable fs UUID ioctl.
>>
>> And above subvolume <-> fs mapping can also be used to resolve missing
>> fd problems.
>> Just adding a fds <-> fs mapping, and try to call commit ioctl on first
>> good fd.
>>
>>>
>>> The typical could be 'btrfs subvol delete -c path/*', ie. deleting from
>>> just one filesystem. If we happen to cross more filesystems, each of
>>> them will get the sync, but possibly more than one. I don't think this
>>> is too serious.
>>
>> Well, after we implemented above user case, some facility can be reused
>> for this case.
>>
>> Thanks,
>> Qu
> 
> OK, I understood the points and will try to make a patch.
> btw, by saying "some facility can be reused", do you mean we can reuse
> fds <-> fs mapping function to other than delete?

Yes, I think there may be some other use cases can benefit from such 
mapping.

Thanks,
Qu

> 
> Thanks,
> Tomohiro
> 
> --
> 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
>