| Message ID | 20200210090201.29979-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync` | expand |
On 2020-02-10 09:02, Qu Wenruo wrote: > Since commit ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem > manual page"), the man page of `btrfs filesystem` shows `sync` > subcommand will wait for all existing orphan subvolumes to be dropped. > > That's not true, even at that commit, `btrfs fi sync` only calls > BTRFS_IOC_SYNC ioctl, which is not that much different from vanilla > sync. > It doesn't wait for orphan subvolumes to be dropped from the very > beginning. > > It's `btrfs subvolume sync` doing the wait work. > > Reported-by: Nikolay Borisov <nborisov@suse.com> > Fixes: ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem > manual page") > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Documentation/btrfs-filesystem.asciidoc | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/Documentation/btrfs-filesystem.asciidoc > b/Documentation/btrfs-filesystem.asciidoc > index 84efaa1a8f8f..3f62a3608cb1 100644 > --- a/Documentation/btrfs-filesystem.asciidoc > +++ b/Documentation/btrfs-filesystem.asciidoc > @@ -253,9 +253,8 @@ show sizes in GiB, or GB with --si > show sizes in TiB, or TB with --si > > *sync* <path>:: > -Force a sync of the filesystem at 'path'. This is done via a special > ioctl and > -will also trigger cleaning of deleted subvolumes. Besides that it's > equivalent > -to the `sync`(1) command. > +Force a sync of the filesystem at 'path'. This should be the same as > vanilla ^^^^^^ As a user I don't like the use of "should" here. Can we not keep the wording of "is equivalent to the `sync(1)` command [but only for the specified btrfs filesystem]"?
On 2020/2/10 下午6:01, Steven Davies wrote: > On 2020-02-10 09:02, Qu Wenruo wrote: >> Since commit ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem >> manual page"), the man page of `btrfs filesystem` shows `sync` >> subcommand will wait for all existing orphan subvolumes to be dropped. >> >> That's not true, even at that commit, `btrfs fi sync` only calls >> BTRFS_IOC_SYNC ioctl, which is not that much different from vanilla >> sync. >> It doesn't wait for orphan subvolumes to be dropped from the very >> beginning. >> >> It's `btrfs subvolume sync` doing the wait work. >> >> Reported-by: Nikolay Borisov <nborisov@suse.com> >> Fixes: ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem >> manual page") >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> Documentation/btrfs-filesystem.asciidoc | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/btrfs-filesystem.asciidoc >> b/Documentation/btrfs-filesystem.asciidoc >> index 84efaa1a8f8f..3f62a3608cb1 100644 >> --- a/Documentation/btrfs-filesystem.asciidoc >> +++ b/Documentation/btrfs-filesystem.asciidoc >> @@ -253,9 +253,8 @@ show sizes in GiB, or GB with --si >> show sizes in TiB, or TB with --si >> >> *sync* <path>:: >> -Force a sync of the filesystem at 'path'. This is done via a special >> ioctl and >> -will also trigger cleaning of deleted subvolumes. Besides that it's >> equivalent >> -to the `sync`(1) command. >> +Force a sync of the filesystem at 'path'. This should be the same as >> vanilla > > ^^^^^^ > As a user I don't like the use of "should" here. Can we not keep the > wording of "is equivalent to the `sync(1)` command [but only for the > specified btrfs filesystem]"? From the view point of code, it's a little different. It also wakes up cleaner thread. But despite that, to end user, it's equivalent. Thanks, Qu
On Mon, Feb 10, 2020 at 05:02:01PM +0800, Qu Wenruo wrote: > Since commit ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem > manual page"), the man page of `btrfs filesystem` shows `sync` > subcommand will wait for all existing orphan subvolumes to be dropped. But this is not what the docs say, nor what the ioctl claims to do. 'trigger cleaning of deleted subvolumes' means that it just starts the process in some way (eg. by waking up the cleaner kthread that does the actual cleaning). For waiting on the subvolumes there's 'btrfs subvol sync' and that works as expected.
On 10/02/2020 16:09, David Sterba wrote: > On Mon, Feb 10, 2020 at 05:02:01PM +0800, Qu Wenruo wrote: >> Since commit ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem >> manual page"), the man page of `btrfs filesystem` shows `sync` >> subcommand will wait for all existing orphan subvolumes to be dropped. > > But this is not what the docs say, nor what the ioctl claims to do. > > 'trigger cleaning of deleted subvolumes' means that it just starts the > process in some way (eg. by waking up the cleaner kthread that does the > actual cleaning). > > For waiting on the subvolumes there's 'btrfs subvol sync' and that works > as expected. I agree. The original wording may be unclear (particularly so for users who may have limited English and be confused by the use of "trigger"), but it does seem to be different from sync(1) and the proposed change is worse. How about: Force a sync of the filesystem at 'path', similar to the `sync`(1) command. In addition, it starts cleaning of deleted subvolumes. To wait for subvolume deletion to complete use the `btrfs subvolume sync` command. SEE ALSO btrfs-subvolume(8)
On 2020/2/11 上午12:09, David Sterba wrote: > On Mon, Feb 10, 2020 at 05:02:01PM +0800, Qu Wenruo wrote: >> Since commit ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem >> manual page"), the man page of `btrfs filesystem` shows `sync` >> subcommand will wait for all existing orphan subvolumes to be dropped. > > But this is not what the docs say, nor what the ioctl claims to do. > > 'trigger cleaning of deleted subvolumes' means that it just starts the > process in some way (eg. by waking up the cleaner kthread that does the > actual cleaning). Then at least we shouldn't really confuse the end user (me included) about the cleanup. In fact the cleaner wake up doesn't really have a user-observable impact. We should at least avoid mentioning such thing. Thanks, Q > > For waiting on the subvolumes there's 'btrfs subvol sync' and that works > as expected. >
On Mon, Feb 10, 2020 at 04:26:22PM +0000, Graham Cobb wrote: > On 10/02/2020 16:09, David Sterba wrote: > > On Mon, Feb 10, 2020 at 05:02:01PM +0800, Qu Wenruo wrote: > >> Since commit ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem > >> manual page"), the man page of `btrfs filesystem` shows `sync` > >> subcommand will wait for all existing orphan subvolumes to be dropped. > > > > But this is not what the docs say, nor what the ioctl claims to do. > > > > 'trigger cleaning of deleted subvolumes' means that it just starts the > > process in some way (eg. by waking up the cleaner kthread that does the > > actual cleaning). > > > > For waiting on the subvolumes there's 'btrfs subvol sync' and that works > > as expected. > > I agree. The original wording may be unclear (particularly so for users > who may have limited English and be confused by the use of "trigger"), > but it does seem to be different from sync(1) and the proposed change is > worse. > > How about: > > Force a sync of the filesystem at 'path', similar to the `sync`(1) > command. In addition, it starts cleaning of deleted subvolumes. To wait > for subvolume deletion to complete use the `btrfs subvolume sync` command. > > SEE ALSO > btrfs-subvolume(8) That's perfect, thanks. I'll update the docs.
On Tue, Feb 11, 2020 at 08:29:22AM +0800, Qu Wenruo wrote: > > > On 2020/2/11 上午12:09, David Sterba wrote: > > On Mon, Feb 10, 2020 at 05:02:01PM +0800, Qu Wenruo wrote: > >> Since commit ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem > >> manual page"), the man page of `btrfs filesystem` shows `sync` > >> subcommand will wait for all existing orphan subvolumes to be dropped. > > > > But this is not what the docs say, nor what the ioctl claims to do. > > > > 'trigger cleaning of deleted subvolumes' means that it just starts the > > process in some way (eg. by waking up the cleaner kthread that does the > > actual cleaning). > > Then at least we shouldn't really confuse the end user (me included) > about the cleanup. Yeah, documentation improvements are welcome. > In fact the cleaner wake up doesn't really have a user-observable impact. The observable impact is that the cleaning starts immediatelly and not after the periodic wakeup. This was the original intention behind 2fad4e83e1259 ("btrfs: wake up transaction thread from SYNC_FS ioctl"). I don't remember exactly, maybe it was on IRC somebody asking for it. > We should at least avoid mentioning such thing. That there's some connection between 'fi sync' and the cleaning should be IMHO mentioned in the docs. The tools provide some commands as building blocks and some as all-in-one, so there should be enough information to let user decide what to do. It's not easy to find the balance between the level of detail and amount of related information, also to maintain consistency accross all documents. As if writing documentation is hard, idk.
diff --git a/Documentation/btrfs-filesystem.asciidoc b/Documentation/btrfs-filesystem.asciidoc index 84efaa1a8f8f..3f62a3608cb1 100644 --- a/Documentation/btrfs-filesystem.asciidoc +++ b/Documentation/btrfs-filesystem.asciidoc @@ -253,9 +253,8 @@ show sizes in GiB, or GB with --si show sizes in TiB, or TB with --si *sync* <path>:: -Force a sync of the filesystem at 'path'. This is done via a special ioctl and -will also trigger cleaning of deleted subvolumes. Besides that it's equivalent -to the `sync`(1) command. +Force a sync of the filesystem at 'path'. This should be the same as vanilla +'sync' command, but only for specified btrfs. *usage* [options] <path> [<path>...]:: Show detailed information about internal filesystem usage. This is supposed to
Since commit ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem manual page"), the man page of `btrfs filesystem` shows `sync` subcommand will wait for all existing orphan subvolumes to be dropped. That's not true, even at that commit, `btrfs fi sync` only calls BTRFS_IOC_SYNC ioctl, which is not that much different from vanilla sync. It doesn't wait for orphan subvolumes to be dropped from the very beginning. It's `btrfs subvolume sync` doing the wait work. Reported-by: Nikolay Borisov <nborisov@suse.com> Fixes: ecd4bb607f35 ("btrfs-progs: docs: enhance btrfs-filesystem manual page") Signed-off-by: Qu Wenruo <wqu@suse.com> --- Documentation/btrfs-filesystem.asciidoc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)