[PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[Qu Wenruo]

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
+'sync' command, but only for specified btrfs.
 
 *usage* [options] <path> [<path>...]::
 Show detailed information about internal filesystem usage. This is supposed to
-- 
2.25.0

Re: [PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[Steven Davies]

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]"?

-- 
Steven Davies

> +'sync' command, but only for specified btrfs.
> 
>  *usage* [options] <path> [<path>...]::
>  Show detailed information about internal filesystem usage. This is 
> supposed to

Re: [PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[Qu Wenruo]

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

Re: [PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[David Sterba]

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.

Re: [PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[Graham Cobb]

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)

Re: [PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[Qu Wenruo]

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.
>

Re: [PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[David Sterba]

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.

Re: [PATCH] btrfs: Doc: Fix the wrong doc about `btrfs filesystem sync`

[David Sterba]

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.