linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] btrfs: fix warn_on for send from readonly mount
@ 2019-12-02  9:44 Anand Jain
  2019-12-02  9:48 ` Nikolay Borisov
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Anand Jain @ 2019-12-02  9:44 UTC (permalink / raw)
  To: linux-btrfs; +Cc: calestyo

We log warning if root::orphan_cleanup_state is not set to
ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
mounted as readonly we skip the orphan items cleanup during the lookup
and root::orphan_cleanup_state remains at the init state 0 instead of
ORPHAN_CLEANUP_DONE (2).

WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
::
RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
::
Call Trace:
::
_btrfs_ioctl_send+0x7b/0x110 [btrfs]
btrfs_ioctl+0x150a/0x2b00 [btrfs]
::
do_vfs_ioctl+0xa9/0x620
? __fget+0xac/0xe0
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x49/0x130
entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reproducer:
  mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
  btrfs subvolume create /btrfs/sv1
  btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
  umount /btrfs && mount -o ro /dev/sdb /btrfs
  btrfs send /btrfs/ss1 -f /tmp/f

Fix this by checking for the expected ORPHAN_CLEANUP_DONE only if the
filesystem is in writable state.

Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/send.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index ae2db5eb1549..e3acec8aa8de 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7085,9 +7085,11 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
 
 	/*
 	 * This is done when we lookup the root, it should already be complete
-	 * by the time we get here.
+	 * by the time we get here, unless the filesystem is readonly where the
+	 * orphan_cleanup_state is never started.
 	 */
-	WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
+	if (!sb_rdonly(file_inode(mnt_file)->i_sb))
+		WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
 
 	/*
 	 * Userspace tools do the checks and warn the user if it's
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs: fix warn_on for send from readonly mount
  2019-12-02  9:44 [PATCH] btrfs: fix warn_on for send from readonly mount Anand Jain
@ 2019-12-02  9:48 ` Nikolay Borisov
  2019-12-02 11:23 ` Filipe Manana
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Nikolay Borisov @ 2019-12-02  9:48 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs; +Cc: calestyo



On 2.12.19 г. 11:44 ч., Anand Jain wrote:
> We log warning if root::orphan_cleanup_state is not set to
> ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
> mounted as readonly we skip the orphan items cleanup during the lookup
> and root::orphan_cleanup_state remains at the init state 0 instead of
> ORPHAN_CLEANUP_DONE (2).
> 
> WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> ::
> RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> ::
> Call Trace:
> ::
> _btrfs_ioctl_send+0x7b/0x110 [btrfs]
> btrfs_ioctl+0x150a/0x2b00 [btrfs]
> ::
> do_vfs_ioctl+0xa9/0x620
> ? __fget+0xac/0xe0
> ksys_ioctl+0x60/0x90
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x49/0x130
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> Reproducer:
>   mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
>   btrfs subvolume create /btrfs/sv1
>   btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
>   umount /btrfs && mount -o ro /dev/sdb /btrfs
>   btrfs send /btrfs/ss1 -f /tmp/f
> 
> Fix this by checking for the expected ORPHAN_CLEANUP_DONE only if the
> filesystem is in writable state.
> 
> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/send.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index ae2db5eb1549..e3acec8aa8de 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -7085,9 +7085,11 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
>  
>  	/*
>  	 * This is done when we lookup the root, it should already be complete
> -	 * by the time we get here.
> +	 * by the time we get here, unless the filesystem is readonly where the
> +	 * orphan_cleanup_state is never started.
>  	 */
> -	WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
> +	if (!sb_rdonly(file_inode(mnt_file)->i_sb))
> +		WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);

nit: WARN_ON(!sb_rdonly() && send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE) ?


>  
>  	/*
>  	 * Userspace tools do the checks and warn the user if it's
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs: fix warn_on for send from readonly mount
  2019-12-02  9:44 [PATCH] btrfs: fix warn_on for send from readonly mount Anand Jain
  2019-12-02  9:48 ` Nikolay Borisov
@ 2019-12-02 11:23 ` Filipe Manana
  2019-12-02 14:07   ` Anand Jain
  2019-12-02 14:24 ` [PATCH v2] " Anand Jain
  2019-12-05 11:39 ` [PATCH v3] " Anand Jain
  3 siblings, 1 reply; 12+ messages in thread
From: Filipe Manana @ 2019-12-02 11:23 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs, Christoph Anton Mitterer

On Mon, Dec 2, 2019 at 9:46 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> We log warning if root::orphan_cleanup_state is not set to
> ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
> mounted as readonly we skip the orphan items cleanup during the lookup
> and root::orphan_cleanup_state remains at the init state 0 instead of
> ORPHAN_CLEANUP_DONE (2).
>
> WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> ::
> RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> ::
> Call Trace:
> ::
> _btrfs_ioctl_send+0x7b/0x110 [btrfs]
> btrfs_ioctl+0x150a/0x2b00 [btrfs]
> ::
> do_vfs_ioctl+0xa9/0x620
> ? __fget+0xac/0xe0
> ksys_ioctl+0x60/0x90
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x49/0x130
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Reproducer:
>   mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
>   btrfs subvolume create /btrfs/sv1
>   btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
>   umount /btrfs && mount -o ro /dev/sdb /btrfs
>   btrfs send /btrfs/ss1 -f /tmp/f
>
> Fix this by checking for the expected ORPHAN_CLEANUP_DONE only if the
> filesystem is in writable state.

I wonder if you know why the warning is there in the first place...

In my opinion we could remove the warning completely because:

1) Having orphan items means we could have files to delete (link count
of 0) and dealing with such cases could make send fail for several
reasons.
    If this happens, it's not longer a problem since the following commit:

    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=46b2f4590aab71d31088a265c86026b1e96c9de4

2) Orphan items used to indicate previously unfinished truncations, in
which case it would lead to send creating corrupt files at the
destination (i_size incorrect and the file filled with zeroes between
real i_size and stale i_size).
    We no longer need to create orphans for truncations since commit:

    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f7e9e8fc792fe2f823ff7d64d23f4363b3f2203a

I think that information needs to be in the changelog. And, as said
before, I think the warning could go away completely.

Thanks.

>
> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/send.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index ae2db5eb1549..e3acec8aa8de 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -7085,9 +7085,11 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
>
>         /*
>          * This is done when we lookup the root, it should already be complete
> -        * by the time we get here.
> +        * by the time we get here, unless the filesystem is readonly where the
> +        * orphan_cleanup_state is never started.
>          */
> -       WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
> +       if (!sb_rdonly(file_inode(mnt_file)->i_sb))
> +               WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
>
>         /*
>          * Userspace tools do the checks and warn the user if it's
> --
> 2.23.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs: fix warn_on for send from readonly mount
  2019-12-02 11:23 ` Filipe Manana
@ 2019-12-02 14:07   ` Anand Jain
  0 siblings, 0 replies; 12+ messages in thread
From: Anand Jain @ 2019-12-02 14:07 UTC (permalink / raw)
  To: fdmanana; +Cc: linux-btrfs, Christoph Anton Mitterer



On 12/2/19 7:23 PM, Filipe Manana wrote:
> On Mon, Dec 2, 2019 at 9:46 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> We log warning if root::orphan_cleanup_state is not set to
>> ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
>> mounted as readonly we skip the orphan items cleanup during the lookup
>> and root::orphan_cleanup_state remains at the init state 0 instead of
>> ORPHAN_CLEANUP_DONE (2).
>>
>> WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
>> ::
>> RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
>> ::
>> Call Trace:
>> ::
>> _btrfs_ioctl_send+0x7b/0x110 [btrfs]
>> btrfs_ioctl+0x150a/0x2b00 [btrfs]
>> ::
>> do_vfs_ioctl+0xa9/0x620
>> ? __fget+0xac/0xe0
>> ksys_ioctl+0x60/0x90
>> __x64_sys_ioctl+0x16/0x20
>> do_syscall_64+0x49/0x130
>> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> Reproducer:
>>    mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
>>    btrfs subvolume create /btrfs/sv1
>>    btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
>>    umount /btrfs && mount -o ro /dev/sdb /btrfs
>>    btrfs send /btrfs/ss1 -f /tmp/f
>>
>> Fix this by checking for the expected ORPHAN_CLEANUP_DONE only if the
>> filesystem is in writable state.
> 
> I wonder if you know why the warning is there in the first place...

  Nope. I didn't go that deep.

> In my opinion we could remove the warning completely because:
 >
> 1) Having orphan items means we could have files to delete (link count
> of 0) and dealing with such cases could make send fail for several
> reasons.
>      If this happens, it's not longer a problem since the following commit:
> 
>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=46b2f4590aab71d31088a265c86026b1e96c9de4
> 
> 2) Orphan items used to indicate previously unfinished truncations, in
> which case it would lead to send creating corrupt files at the
> destination (i_size incorrect and the file filled with zeroes between
> real i_size and stale i_size).
>      We no longer need to create orphans for truncations since commit:
> 
>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f7e9e8fc792fe2f823ff7d64d23f4363b3f2203a
> 
> I think that information needs to be in the changelog. And, as said
> before, I think the warning could go away completely.

  Makes sense. Will send v2 with it.

Thanks, Anand

> Thanks.
> 
>>
>> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/send.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>> index ae2db5eb1549..e3acec8aa8de 100644
>> --- a/fs/btrfs/send.c
>> +++ b/fs/btrfs/send.c
>> @@ -7085,9 +7085,11 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
>>
>>          /*
>>           * This is done when we lookup the root, it should already be complete
>> -        * by the time we get here.
>> +        * by the time we get here, unless the filesystem is readonly where the
>> +        * orphan_cleanup_state is never started.
>>           */
>> -       WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
>> +       if (!sb_rdonly(file_inode(mnt_file)->i_sb))
>> +               WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
>>
>>          /*
>>           * Userspace tools do the checks and warn the user if it's
>> --
>> 2.23.0
>>
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] btrfs: fix warn_on for send from readonly mount
  2019-12-02  9:44 [PATCH] btrfs: fix warn_on for send from readonly mount Anand Jain
  2019-12-02  9:48 ` Nikolay Borisov
  2019-12-02 11:23 ` Filipe Manana
@ 2019-12-02 14:24 ` Anand Jain
  2019-12-02 15:42   ` Filipe Manana
  2019-12-05 11:39 ` [PATCH v3] " Anand Jain
  3 siblings, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-12-02 14:24 UTC (permalink / raw)
  To: linux-btrfs

We log warning if root::orphan_cleanup_state is not set to
ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
mounted as readonly we skip the orphan items cleanup during the lookup
and root::orphan_cleanup_state remains at the init state 0 instead of
ORPHAN_CLEANUP_DONE (2).

WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
::
RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
::
Call Trace:
::
_btrfs_ioctl_send+0x7b/0x110 [btrfs]
btrfs_ioctl+0x150a/0x2b00 [btrfs]
::
do_vfs_ioctl+0xa9/0x620
? __fget+0xac/0xe0
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x49/0x130
entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reproducer:
  mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
  btrfs subvolume create /btrfs/sv1
  btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
  umount /btrfs && mount -o ro /dev/sdb /btrfs
  btrfs send /btrfs/ss1 -f /tmp/f

Fix this by removing the warn_on completely because:

1) Having orphan items means we could have files to delete (link count
of 0) and dealing with such cases could make send fail for several
reasons.
   If this happens, it's not longer a problem since the following
commit:
   46b2f4590aab71d31088a265c86026b1e96c9de4
   Btrfs: fix send failure when root has deleted files still open

2) Orphan items used to indicate previously unfinished truncations, in
which case it would lead to send creating corrupt files at the
destination (i_size incorrect and the file filled with zeroes between
real i_size and stale i_size).
   We no longer need to create orphans for truncations since commit:
   f7e9e8fc792fe2f823ff7d64d23f4363b3f2203a
   Btrfs: stop creating orphan items for truncate

Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
Suggested-by: Filipe Manana <fdmanana@gmail.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2:
 Remove WARN_ON() completely.

 fs/btrfs/send.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index ae2db5eb1549..091e5bc8c7ea 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7084,12 +7084,6 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
 	spin_unlock(&send_root->root_item_lock);
 
 	/*
-	 * This is done when we lookup the root, it should already be complete
-	 * by the time we get here.
-	 */
-	WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
-
-	/*
 	 * Userspace tools do the checks and warn the user if it's
 	 * not RO.
 	 */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] btrfs: fix warn_on for send from readonly mount
  2019-12-02 14:24 ` [PATCH v2] " Anand Jain
@ 2019-12-02 15:42   ` Filipe Manana
  2019-12-02 23:59     ` Anand Jain
  0 siblings, 1 reply; 12+ messages in thread
From: Filipe Manana @ 2019-12-02 15:42 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Mon, Dec 2, 2019 at 2:26 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> We log warning if root::orphan_cleanup_state is not set to
> ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
> mounted as readonly we skip the orphan items cleanup during the lookup
> and root::orphan_cleanup_state remains at the init state 0 instead of
> ORPHAN_CLEANUP_DONE (2).
>
> WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> ::
> RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> ::
> Call Trace:
> ::
> _btrfs_ioctl_send+0x7b/0x110 [btrfs]
> btrfs_ioctl+0x150a/0x2b00 [btrfs]
> ::
> do_vfs_ioctl+0xa9/0x620
> ? __fget+0xac/0xe0
> ksys_ioctl+0x60/0x90
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x49/0x130
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Reproducer:
>   mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
>   btrfs subvolume create /btrfs/sv1
>   btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
>   umount /btrfs && mount -o ro /dev/sdb /btrfs
>   btrfs send /btrfs/ss1 -f /tmp/f
>
> Fix this by removing the warn_on completely because:
>
> 1) Having orphan items means we could have files to delete (link count
> of 0) and dealing with such cases could make send fail for several
> reasons.
>    If this happens, it's not longer a problem since the following
> commit:
>    46b2f4590aab71d31088a265c86026b1e96c9de4
>    Btrfs: fix send failure when root has deleted files still open

The convention for mentioning commits is
first_12_or_slighly_more_hash_characters ("subject").
scripts/checkpatch.pl warns about it, and given this has been around
for years, you should already be familiar with it.

>
> 2) Orphan items used to indicate previously unfinished truncations, in
> which case it would lead to send creating corrupt files at the
> destination (i_size incorrect and the file filled with zeroes between
> real i_size and stale i_size).
>    We no longer need to create orphans for truncations since commit:
>    f7e9e8fc792fe2f823ff7d64d23f4363b3f2203a
>    Btrfs: stop creating orphan items for truncate

And I didn't expect you to literally copy-paste what I wrote before.
For a changelog we want something better written, organized and more
detailed then an informal e-mail reply, like this:

"
The warning exists because having orphanized inodes could confuse send
and cause it to fail or produce incorrect streams.
The two cases that would cause problems were:

1) Inodes that were unlinked - these are orphanized and remain with a
link count of 0, having no references (names).
   These caused send operations to fail because it expected to always
find at least one path for an inode.
   This is no longe a problem since send is now able to deal with such
inodes since
   commit 46b2f4590aab ("Btrfs: fix send failure when root has deleted
files still open") and treats them as having
   been completely removed (the state after a orphan cleanup is performed).

2) Inodes that were in the process of being truncated. These resulted
in send not knowing about the truncation
    and potentially issue write operations full of zeroes for the
range from the new file size to the old file size.
    This is no longer a problem because we no longer create orphan
items for truncations since
    commit  f7e9e8fc792f ("Btrfs: stop creating orphan items for truncate").

In other words the warning was there to provide a clue in case
something went wrong. Instead of being a warning
against the root's "->orphan_cleanup_state" value, it could have been
more accurate by checking if there were actually
any orphan items, and then issue a warning only if any exists, but
that would be more expensive to check.
Since orphanized inodes no longer cause problems for send, just remove
the warning.
"

>
> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
> Suggested-by: Filipe Manana <fdmanana@gmail.com>

Also s/@gmail.com/@suse.com/ (preferable).

Thanks.

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2:
>  Remove WARN_ON() completely.
>
>  fs/btrfs/send.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index ae2db5eb1549..091e5bc8c7ea 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -7084,12 +7084,6 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
>         spin_unlock(&send_root->root_item_lock);
>
>         /*
> -        * This is done when we lookup the root, it should already be complete
> -        * by the time we get here.
> -        */
> -       WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
> -
> -       /*
>          * Userspace tools do the checks and warn the user if it's
>          * not RO.
>          */
> --
> 1.8.3.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] btrfs: fix warn_on for send from readonly mount
  2019-12-02 15:42   ` Filipe Manana
@ 2019-12-02 23:59     ` Anand Jain
  2019-12-03 11:45       ` Filipe Manana
  0 siblings, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-12-02 23:59 UTC (permalink / raw)
  To: fdmanana; +Cc: linux-btrfs

On 12/2/19 11:42 PM, Filipe Manana wrote:
> On Mon, Dec 2, 2019 at 2:26 PM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> We log warning if root::orphan_cleanup_state is not set to
>> ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
>> mounted as readonly we skip the orphan items cleanup during the lookup
>> and root::orphan_cleanup_state remains at the init state 0 instead of
>> ORPHAN_CLEANUP_DONE (2).
>>
>> WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
>> ::
>> RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
>> ::
>> Call Trace:
>> ::
>> _btrfs_ioctl_send+0x7b/0x110 [btrfs]
>> btrfs_ioctl+0x150a/0x2b00 [btrfs]
>> ::
>> do_vfs_ioctl+0xa9/0x620
>> ? __fget+0xac/0xe0
>> ksys_ioctl+0x60/0x90
>> __x64_sys_ioctl+0x16/0x20
>> do_syscall_64+0x49/0x130
>> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> Reproducer:
>>    mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
>>    btrfs subvolume create /btrfs/sv1
>>    btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
>>    umount /btrfs && mount -o ro /dev/sdb /btrfs
>>    btrfs send /btrfs/ss1 -f /tmp/f
>>
>> Fix this by removing the warn_on completely because:
>>
>> 1) Having orphan items means we could have files to delete (link count
>> of 0) and dealing with such cases could make send fail for several
>> reasons.
>>     If this happens, it's not longer a problem since the following
>> commit:
>>     46b2f4590aab71d31088a265c86026b1e96c9de4
>>     Btrfs: fix send failure when root has deleted files still open
> 
> The convention for mentioning commits is
> first_12_or_slighly_more_hash_characters ("subject").
> scripts/checkpatch.pl warns about it, and given this has been around
> for years, you should already be familiar with it.
> 
>>
>> 2) Orphan items used to indicate previously unfinished truncations, in
>> which case it would lead to send creating corrupt files at the
>> destination (i_size incorrect and the file filled with zeroes between
>> real i_size and stale i_size).
>>     We no longer need to create orphans for truncations since commit:
>>     f7e9e8fc792fe2f823ff7d64d23f4363b3f2203a
>>     Btrfs: stop creating orphan items for truncate
> 
> And I didn't expect you to literally copy-paste what I wrote before.
> For a changelog we want something better written, organized and more
> detailed then an informal e-mail reply, like this:
> 
> "
> The warning exists because having orphanized inodes could confuse send
> and cause it to fail or produce incorrect streams.
> The two cases that would cause problems were:
> 
> 1) Inodes that were unlinked - these are orphanized and remain with a
> link count of 0, having no references (names).
>     These caused send operations to fail because it expected to always
> find at least one path for an inode.
>     This is no longe a problem since send is now able to deal with such
> inodes since
>     commit 46b2f4590aab ("Btrfs: fix send failure when root has deleted
> files still open") and treats them as having
>     been completely removed (the state after a orphan cleanup is performed).
> 
> 2) Inodes that were in the process of being truncated. These resulted
> in send not knowing about the truncation
>      and potentially issue write operations full of zeroes for the
> range from the new file size to the old file size.
>      This is no longer a problem because we no longer create orphan
> items for truncations since
>      commit  f7e9e8fc792f ("Btrfs: stop creating orphan items for truncate")..
> 
> In other words the warning was there to provide a clue in case
> something went wrong. Instead of being a warning
> against the root's "->orphan_cleanup_state" value, it could have been
> more accurate by checking if there were actually
> any orphan items, and then issue a warning only if any exists, but
> that would be more expensive to check.
> Since orphanized inodes no longer cause problems for send, just remove
> the warning.
> "

  Ah. Generally your commits has the best change logs. Its very hard
  to either match or satisfy your level. ;-). But I am trying and not
  there yet.

Thanks. Anand

>>
>> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
>> Suggested-by: Filipe Manana <fdmanana@gmail.com>
> 
> Also s/@gmail.com/@suse.com/ (preferable).
> 
> Thanks.
> 
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2:
>>   Remove WARN_ON() completely.
>>
>>   fs/btrfs/send.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>> index ae2db5eb1549..091e5bc8c7ea 100644
>> --- a/fs/btrfs/send.c
>> +++ b/fs/btrfs/send.c
>> @@ -7084,12 +7084,6 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
>>          spin_unlock(&send_root->root_item_lock);
>>
>>          /*
>> -        * This is done when we lookup the root, it should already be complete
>> -        * by the time we get here.
>> -        */
>> -       WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
>> -
>> -       /*
>>           * Userspace tools do the checks and warn the user if it's
>>           * not RO.
>>           */
>> --
>> 1.8.3.1
>>
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] btrfs: fix warn_on for send from readonly mount
  2019-12-02 23:59     ` Anand Jain
@ 2019-12-03 11:45       ` Filipe Manana
  0 siblings, 0 replies; 12+ messages in thread
From: Filipe Manana @ 2019-12-03 11:45 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Mon, Dec 2, 2019 at 11:59 PM Anand Jain <anandsuveer@gmail.com> wrote:
>
> On 12/2/19 11:42 PM, Filipe Manana wrote:
> > On Mon, Dec 2, 2019 at 2:26 PM Anand Jain <anand.jain@oracle.com> wrote:
> >>
> >> We log warning if root::orphan_cleanup_state is not set to
> >> ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
> >> mounted as readonly we skip the orphan items cleanup during the lookup
> >> and root::orphan_cleanup_state remains at the init state 0 instead of
> >> ORPHAN_CLEANUP_DONE (2).
> >>
> >> WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> >> ::
> >> RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> >> ::
> >> Call Trace:
> >> ::
> >> _btrfs_ioctl_send+0x7b/0x110 [btrfs]
> >> btrfs_ioctl+0x150a/0x2b00 [btrfs]
> >> ::
> >> do_vfs_ioctl+0xa9/0x620
> >> ? __fget+0xac/0xe0
> >> ksys_ioctl+0x60/0x90
> >> __x64_sys_ioctl+0x16/0x20
> >> do_syscall_64+0x49/0x130
> >> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >>
> >> Reproducer:
> >>    mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
> >>    btrfs subvolume create /btrfs/sv1
> >>    btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
> >>    umount /btrfs && mount -o ro /dev/sdb /btrfs
> >>    btrfs send /btrfs/ss1 -f /tmp/f
> >>
> >> Fix this by removing the warn_on completely because:
> >>
> >> 1) Having orphan items means we could have files to delete (link count
> >> of 0) and dealing with such cases could make send fail for several
> >> reasons.
> >>     If this happens, it's not longer a problem since the following
> >> commit:
> >>     46b2f4590aab71d31088a265c86026b1e96c9de4
> >>     Btrfs: fix send failure when root has deleted files still open
> >
> > The convention for mentioning commits is
> > first_12_or_slighly_more_hash_characters ("subject").
> > scripts/checkpatch.pl warns about it, and given this has been around
> > for years, you should already be familiar with it.
> >
> >>
> >> 2) Orphan items used to indicate previously unfinished truncations, in
> >> which case it would lead to send creating corrupt files at the
> >> destination (i_size incorrect and the file filled with zeroes between
> >> real i_size and stale i_size).
> >>     We no longer need to create orphans for truncations since commit:
> >>     f7e9e8fc792fe2f823ff7d64d23f4363b3f2203a
> >>     Btrfs: stop creating orphan items for truncate
> >
> > And I didn't expect you to literally copy-paste what I wrote before.
> > For a changelog we want something better written, organized and more
> > detailed then an informal e-mail reply, like this:
> >
> > "
> > The warning exists because having orphanized inodes could confuse send
> > and cause it to fail or produce incorrect streams.
> > The two cases that would cause problems were:
> >
> > 1) Inodes that were unlinked - these are orphanized and remain with a
> > link count of 0, having no references (names).
> >     These caused send operations to fail because it expected to always
> > find at least one path for an inode.
> >     This is no longe a problem since send is now able to deal with such
> > inodes since
> >     commit 46b2f4590aab ("Btrfs: fix send failure when root has deleted
> > files still open") and treats them as having
> >     been completely removed (the state after a orphan cleanup is performed).
> >
> > 2) Inodes that were in the process of being truncated. These resulted
> > in send not knowing about the truncation
> >      and potentially issue write operations full of zeroes for the
> > range from the new file size to the old file size.
> >      This is no longer a problem because we no longer create orphan
> > items for truncations since
> >      commit  f7e9e8fc792f ("Btrfs: stop creating orphan items for truncate")..
> >
> > In other words the warning was there to provide a clue in case
> > something went wrong. Instead of being a warning
> > against the root's "->orphan_cleanup_state" value, it could have been
> > more accurate by checking if there were actually
> > any orphan items, and then issue a warning only if any exists, but
> > that would be more expensive to check.
> > Since orphanized inodes no longer cause problems for send, just remove
> > the warning.
> > "
>
>   Ah. Generally your commits has the best change logs. Its very hard
>   to either match or satisfy your level. ;-). But I am trying and not
>   there yet.

Thank you, you flatter me.

If you send a new version, a link tag could also be added:

Link: https://lore.kernel.org/linux-btrfs/21cb5e8d059f6e1496a903fa7bfc0a297e2f5370.camel@scientia.net/

thanks

>
> Thanks. Anand
>
> >>
> >> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
> >> Suggested-by: Filipe Manana <fdmanana@gmail.com>
> >
> > Also s/@gmail.com/@suse.com/ (preferable).
> >
> > Thanks.
> >
> >> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> >> ---
> >> v2:
> >>   Remove WARN_ON() completely.
> >>
> >>   fs/btrfs/send.c | 6 ------
> >>   1 file changed, 6 deletions(-)
> >>
> >> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> >> index ae2db5eb1549..091e5bc8c7ea 100644
> >> --- a/fs/btrfs/send.c
> >> +++ b/fs/btrfs/send.c
> >> @@ -7084,12 +7084,6 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
> >>          spin_unlock(&send_root->root_item_lock);
> >>
> >>          /*
> >> -        * This is done when we lookup the root, it should already be complete
> >> -        * by the time we get here.
> >> -        */
> >> -       WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
> >> -
> >> -       /*
> >>           * Userspace tools do the checks and warn the user if it's
> >>           * not RO.
> >>           */
> >> --
> >> 1.8.3.1
> >>
> >
> >
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] btrfs: fix warn_on for send from readonly mount
  2019-12-02  9:44 [PATCH] btrfs: fix warn_on for send from readonly mount Anand Jain
                   ` (2 preceding siblings ...)
  2019-12-02 14:24 ` [PATCH v2] " Anand Jain
@ 2019-12-05 11:39 ` Anand Jain
  2019-12-05 11:43   ` Filipe Manana
  3 siblings, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-12-05 11:39 UTC (permalink / raw)
  To: linux-btrfs

We log warning if root::orphan_cleanup_state is not set to
ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
mounted as readonly we skip the orphan items cleanup during the lookup
and root::orphan_cleanup_state remains at the init state 0 instead of
ORPHAN_CLEANUP_DONE (2). So during send in btrfs_ioctl_send() we hit
the warning as below.

  WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);

WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
::
RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
::
Call Trace:
::
_btrfs_ioctl_send+0x7b/0x110 [btrfs]
btrfs_ioctl+0x150a/0x2b00 [btrfs]
::
do_vfs_ioctl+0xa9/0x620
? __fget+0xac/0xe0
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x49/0x130
entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reproducer:
  mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
  btrfs subvolume create /btrfs/sv1
  btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
  umount /btrfs && mount -o ro /dev/sdb /btrfs
  btrfs send /btrfs/ss1 -f /tmp/f

The warning exists because having orphan inodes could confuse send
and cause it to fail or produce incorrect streams.
The two cases that would cause such send failures, which are already
fixed are:

1) Inodes that were unlinked - these are orphanized and remain with a link
count of 0. These caused send operations to fail because it expected to
always find at least one path for an inode. However this is no longer a
problem since send is now able to deal with such inodes since commit
46b2f4590aab ("Btrfs: fix send failure when root has deleted files still
open") and treats them as having been completely removed (the state after
a orphan cleanup is performed).

2) Inodes that were in the process of being truncated. These resulted in
send not knowing about the truncation and potentially issue write
operations full of zeroes for the range from the new file size to the old
file size. This is no longer a problem because we no longer create orphan
items for truncation since commit f7e9e8fc792f ("Btrfs: stop creating
orphan items for truncate").

As such before these commits, the WARN_ON here provided a clue in case
something went wrong. Instead of being a warning against the
root::orphan_cleanup_state value, it could have been more accurate by
checking if there were actually any orphan items, and then issue a warning
only if any exists, but that would be more expensive to check. Since
orphanized inodes no longer cause problems for send, just remove the warning.

Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
Link: https://lore.kernel.org/linux-btrfs/21cb5e8d059f6e1496a903fa7bfc0a297e2f5370.camel@scientia.net/
Suggested-by: Filipe Manana <fdmanana@gmail.com>
[ Remove warn_on() part, and its reasoning ]
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: Commit log updated.
v2: Remove WARN_ON() completely.
 fs/btrfs/send.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index ae2db5eb1549..091e5bc8c7ea 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7084,12 +7084,6 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
 	spin_unlock(&send_root->root_item_lock);
 
 	/*
-	 * This is done when we lookup the root, it should already be complete
-	 * by the time we get here.
-	 */
-	WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
-
-	/*
 	 * Userspace tools do the checks and warn the user if it's
 	 * not RO.
 	 */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] btrfs: fix warn_on for send from readonly mount
  2019-12-05 11:39 ` [PATCH v3] " Anand Jain
@ 2019-12-05 11:43   ` Filipe Manana
  2019-12-05 11:45     ` Anand Jain
  0 siblings, 1 reply; 12+ messages in thread
From: Filipe Manana @ 2019-12-05 11:43 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Thu, Dec 5, 2019 at 11:39 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> We log warning if root::orphan_cleanup_state is not set to
> ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
> mounted as readonly we skip the orphan items cleanup during the lookup
> and root::orphan_cleanup_state remains at the init state 0 instead of
> ORPHAN_CLEANUP_DONE (2). So during send in btrfs_ioctl_send() we hit
> the warning as below.
>
>   WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
>
> WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> ::
> RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
> ::
> Call Trace:
> ::
> _btrfs_ioctl_send+0x7b/0x110 [btrfs]
> btrfs_ioctl+0x150a/0x2b00 [btrfs]
> ::
> do_vfs_ioctl+0xa9/0x620
> ? __fget+0xac/0xe0
> ksys_ioctl+0x60/0x90
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x49/0x130
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Reproducer:
>   mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
>   btrfs subvolume create /btrfs/sv1
>   btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
>   umount /btrfs && mount -o ro /dev/sdb /btrfs
>   btrfs send /btrfs/ss1 -f /tmp/f
>
> The warning exists because having orphan inodes could confuse send
> and cause it to fail or produce incorrect streams.
> The two cases that would cause such send failures, which are already
> fixed are:
>
> 1) Inodes that were unlinked - these are orphanized and remain with a link
> count of 0. These caused send operations to fail because it expected to
> always find at least one path for an inode. However this is no longer a
> problem since send is now able to deal with such inodes since commit
> 46b2f4590aab ("Btrfs: fix send failure when root has deleted files still
> open") and treats them as having been completely removed (the state after
> a orphan cleanup is performed).
>
> 2) Inodes that were in the process of being truncated. These resulted in
> send not knowing about the truncation and potentially issue write
> operations full of zeroes for the range from the new file size to the old
> file size. This is no longer a problem because we no longer create orphan
> items for truncation since commit f7e9e8fc792f ("Btrfs: stop creating
> orphan items for truncate").
>
> As such before these commits, the WARN_ON here provided a clue in case
> something went wrong. Instead of being a warning against the
> root::orphan_cleanup_state value, it could have been more accurate by
> checking if there were actually any orphan items, and then issue a warning
> only if any exists, but that would be more expensive to check. Since
> orphanized inodes no longer cause problems for send, just remove the warning.
>
> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
> Link: https://lore.kernel.org/linux-btrfs/21cb5e8d059f6e1496a903fa7bfc0a297e2f5370.camel@scientia.net/
> Suggested-by: Filipe Manana <fdmanana@gmail.com>

s/gmail.com/suse.com/
(David can probably do that when he picks the patch)

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks

> [ Remove warn_on() part, and its reasoning ]
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v3: Commit log updated.
> v2: Remove WARN_ON() completely.
>  fs/btrfs/send.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index ae2db5eb1549..091e5bc8c7ea 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -7084,12 +7084,6 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
>         spin_unlock(&send_root->root_item_lock);
>
>         /*
> -        * This is done when we lookup the root, it should already be complete
> -        * by the time we get here.
> -        */
> -       WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
> -
> -       /*
>          * Userspace tools do the checks and warn the user if it's
>          * not RO.
>          */
> --
> 1.8.3.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] btrfs: fix warn_on for send from readonly mount
  2019-12-05 11:43   ` Filipe Manana
@ 2019-12-05 11:45     ` Anand Jain
  2019-12-10 16:51       ` David Sterba
  0 siblings, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-12-05 11:45 UTC (permalink / raw)
  To: fdmanana; +Cc: linux-btrfs



On 5/12/19 7:43 PM, Filipe Manana wrote:
> On Thu, Dec 5, 2019 at 11:39 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> We log warning if root::orphan_cleanup_state is not set to
>> ORPHAN_CLEANUP_DONE in btrfs_ioctl_send(). However if the filesystem is
>> mounted as readonly we skip the orphan items cleanup during the lookup
>> and root::orphan_cleanup_state remains at the init state 0 instead of
>> ORPHAN_CLEANUP_DONE (2). So during send in btrfs_ioctl_send() we hit
>> the warning as below.
>>
>>    WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
>>
>> WARNING: CPU: 0 PID: 2616 at /Volumes/ws/btrfs-devel/fs/btrfs/send.c:7090 btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
>> ::
>> RIP: 0010:btrfs_ioctl_send+0xb2f/0x18c0 [btrfs]
>> ::
>> Call Trace:
>> ::
>> _btrfs_ioctl_send+0x7b/0x110 [btrfs]
>> btrfs_ioctl+0x150a/0x2b00 [btrfs]
>> ::
>> do_vfs_ioctl+0xa9/0x620
>> ? __fget+0xac/0xe0
>> ksys_ioctl+0x60/0x90
>> __x64_sys_ioctl+0x16/0x20
>> do_syscall_64+0x49/0x130
>> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> Reproducer:
>>    mkfs.btrfs -fq /dev/sdb && mount /dev/sdb /btrfs
>>    btrfs subvolume create /btrfs/sv1
>>    btrfs subvolume snapshot -r /btrfs/sv1 /btrfs/ss1
>>    umount /btrfs && mount -o ro /dev/sdb /btrfs
>>    btrfs send /btrfs/ss1 -f /tmp/f
>>
>> The warning exists because having orphan inodes could confuse send
>> and cause it to fail or produce incorrect streams.
>> The two cases that would cause such send failures, which are already
>> fixed are:
>>
>> 1) Inodes that were unlinked - these are orphanized and remain with a link
>> count of 0. These caused send operations to fail because it expected to
>> always find at least one path for an inode. However this is no longer a
>> problem since send is now able to deal with such inodes since commit
>> 46b2f4590aab ("Btrfs: fix send failure when root has deleted files still
>> open") and treats them as having been completely removed (the state after
>> a orphan cleanup is performed).
>>
>> 2) Inodes that were in the process of being truncated. These resulted in
>> send not knowing about the truncation and potentially issue write
>> operations full of zeroes for the range from the new file size to the old
>> file size. This is no longer a problem because we no longer create orphan
>> items for truncation since commit f7e9e8fc792f ("Btrfs: stop creating
>> orphan items for truncate").
>>
>> As such before these commits, the WARN_ON here provided a clue in case
>> something went wrong. Instead of being a warning against the
>> root::orphan_cleanup_state value, it could have been more accurate by
>> checking if there were actually any orphan items, and then issue a warning
>> only if any exists, but that would be more expensive to check. Since
>> orphanized inodes no longer cause problems for send, just remove the warning.
>>
>> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
>> Link: https://lore.kernel.org/linux-btrfs/21cb5e8d059f6e1496a903fa7bfc0a297e2f5370.camel@scientia.net/
>> Suggested-by: Filipe Manana <fdmanana@gmail.com>
> 
> s/gmail.com/suse.com/
> (David can probably do that when he picks the patch)
> 
  Oh. Sorry I missed it. Thanks, Anand

> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Thanks
> 
>> [ Remove warn_on() part, and its reasoning ]
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v3: Commit log updated.
>> v2: Remove WARN_ON() completely.
>>   fs/btrfs/send.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>> index ae2db5eb1549..091e5bc8c7ea 100644
>> --- a/fs/btrfs/send.c
>> +++ b/fs/btrfs/send.c
>> @@ -7084,12 +7084,6 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
>>          spin_unlock(&send_root->root_item_lock);
>>
>>          /*
>> -        * This is done when we lookup the root, it should already be complete
>> -        * by the time we get here.
>> -        */
>> -       WARN_ON(send_root->orphan_cleanup_state != ORPHAN_CLEANUP_DONE);
>> -
>> -       /*
>>           * Userspace tools do the checks and warn the user if it's
>>           * not RO.
>>           */
>> --
>> 1.8.3.1
>>
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] btrfs: fix warn_on for send from readonly mount
  2019-12-05 11:45     ` Anand Jain
@ 2019-12-10 16:51       ` David Sterba
  0 siblings, 0 replies; 12+ messages in thread
From: David Sterba @ 2019-12-10 16:51 UTC (permalink / raw)
  To: Anand Jain; +Cc: fdmanana, linux-btrfs

On Thu, Dec 05, 2019 at 07:45:10PM +0800, Anand Jain wrote:
> On 5/12/19 7:43 PM, Filipe Manana wrote:
> > On Thu, Dec 5, 2019 at 11:39 AM Anand Jain <anand.jain@oracle.com> wrote:
> >> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
> >> Link: https://lore.kernel.org/linux-btrfs/21cb5e8d059f6e1496a903fa7bfc0a297e2f5370.camel@scientia.net/
> >> Suggested-by: Filipe Manana <fdmanana@gmail.com>
> > 
> > s/gmail.com/suse.com/
> > (David can probably do that when he picks the patch)
> > 
>   Oh. Sorry I missed it. Thanks, Anand

Fixed and added to misc-next, thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-12-10 16:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02  9:44 [PATCH] btrfs: fix warn_on for send from readonly mount Anand Jain
2019-12-02  9:48 ` Nikolay Borisov
2019-12-02 11:23 ` Filipe Manana
2019-12-02 14:07   ` Anand Jain
2019-12-02 14:24 ` [PATCH v2] " Anand Jain
2019-12-02 15:42   ` Filipe Manana
2019-12-02 23:59     ` Anand Jain
2019-12-03 11:45       ` Filipe Manana
2019-12-05 11:39 ` [PATCH v3] " Anand Jain
2019-12-05 11:43   ` Filipe Manana
2019-12-05 11:45     ` Anand Jain
2019-12-10 16:51       ` David Sterba

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).