| Message ID | 20191202094450.1377-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix warn_on for send from readonly mount | expand |
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 >
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 >
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 >> > >
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
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(-)