[v3] btrfs: fix warn_on for send from readonly mount
diff mbox series

Message ID 20191205113907.8269-1-anand.jain@oracle.com
State New
Headers show
Series
  • [v3] btrfs: fix warn_on for send from readonly mount
Related show

Commit Message

Anand Jain Dec. 5, 2019, 11:39 a.m. UTC
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(-)

Comments

Filipe Manana Dec. 5, 2019, 11:43 a.m. UTC | #1
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
>
Anand Jain Dec. 5, 2019, 11:45 a.m. UTC | #2
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
>>
> 
>
David Sterba Dec. 10, 2019, 4:51 p.m. UTC | #3
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.

Patch
diff mbox series

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