Re: [PATCH v2 04/34] md: port block device access to file

[Jan Kara <jack@suse.cz>]

On Mon 15-04-24 23:11:50, Ming Lei wrote:
> On Mon, Apr 15, 2024 at 04:53:42PM +0200, Christian Brauner wrote:
> > On Mon, Apr 15, 2024 at 10:35:53PM +0800, Ming Lei wrote:
> > > On Mon, Apr 15, 2024 at 02:35:17PM +0200, Christian Brauner wrote:
> > > > On Mon, Apr 15, 2024 at 05:26:19PM +0800, Ming Lei wrote:
> > > > > Hello,
> > > > > 
> > > > > On Tue, Jan 23, 2024 at 02:26:21PM +0100, Christian Brauner wrote:
> > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > > > ---
> > > > > >  drivers/md/dm.c               | 23 +++++++++++++----------
> > > > > >  drivers/md/md.c               | 12 ++++++------
> > > > > >  drivers/md/md.h               |  2 +-
> > > > > >  include/linux/device-mapper.h |  2 +-
> > > > > >  4 files changed, 21 insertions(+), 18 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > > > > > index 8dcabf84d866..87de5b5682ad 100644
> > > > > > --- a/drivers/md/dm.c
> > > > > > +++ b/drivers/md/dm.c
> > > > > 
> > > > > ...
> > > > > 
> > > > > > @@ -775,7 +778,7 @@ static void close_table_device(struct table_device *td, struct mapped_device *md
> > > > > >  {
> > > > > >  	if (md->disk->slave_dir)
> > > > > >  		bd_unlink_disk_holder(td->dm_dev.bdev, md->disk);
> > > > > > -	bdev_release(td->dm_dev.bdev_handle);
> > > > > > +	fput(td->dm_dev.bdev_file);
> > > > > 
> > > > > The above change caused regression on 'dmsetup remove_all'.
> > > > > 
> > > > > blkdev_release() is delayed because of fput(), so dm_lock_for_deletion
> > > > > returns -EBUSY, then this dm disk is skipped in remove_all().
> > > > > 
> > > > > Force to mark DMF_DEFERRED_REMOVE might solve it, but need our device
> > > > > mapper guys to check if it is safe.
> > > > > 
> > > > > Or other better solution?
> > > > 
> > > > Yeah, I think there is. You can just switch all fput() instances in
> > > > device mapper to bdev_fput() which is mainline now. This will yield the
> > > > device and make it able to be reclaimed. Should be as simple as the
> > > > patch below. Could you test this and send a patch based on this (I'm on
> > > > a prolonged vacation so I don't have time right now.):
> > > 
> > > Unfortunately it doesn't work.
> > > 
> > > Here the problem is that blkdev_release() is delayed, which changes
> > > 'dmsetup remove_all' behavior, and causes that some of dm disks aren't
> > > removed.
> > > 
> > > Please see dm_lock_for_deletion() and dm_blk_open()/dm_blk_close().
> > 
> > So you really need blkdev_release() itself to be synchronous? Groan, in
> 
> At least the current dm implementation relies on this way sort of, and
> it could be addressed by forcing to mark DMF_DEFERRED_REMOVE in
> remove_all().
> 
> > that case use __fput_sync() instead of fput() which ensures that this
> > file is closed synchronously.
> 
> I tried __fput_sync(), but the following panic is caused:
> 
> [  113.486522] ------------[ cut here ]------------
> [  113.486524] kernel BUG at fs/file_table.c:453!
> [  113.486531] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [  113.488878] CPU: 6 PID: 1919 Comm: dmsetup Kdump: loaded Not tainted 5.14.0+ #23

Wait, how come this is 5.14 kernel? Apparently you're crashing on:

BUG_ON(!(task->flags & PF_KTHREAD));

but that is not present in current upstream (BUG_ON was removed in 6.6-rc1
by commit 021a160abf62c).

								Honza

> [  113.490114] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc37 04/01/2014
> [  113.491661] RIP: 0010:__fput_sync+0x25/0x30
> [  113.492562] Code: 90 90 90 90 90 0f 1f 44 00 00 f0 48 ff 4f 38 75 14 65 48 8b 04 25 40 25 03 00 f6 40 36 20 74 0a e9 20 fd ff ff c3 cc cc cc cc <0f0
> [  113.493926] RSP: 0018:ffffb76581003c20 EFLAGS: 00010246
> [  113.494220] RAX: ffff92eca6ef8000 RBX: ffff92ed176c3c18 RCX: 000000008080007c
> [  113.494632] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff92ec844cac00
> [  113.495033] RBP: ffff92ed176c3c00 R08: 0000000000000001 R09: 0000000000000000
> [  113.495378] R10: ffffb76581003b00 R11: ffffb76581003b68 R12: ffff92ec8fccec20
> [  113.495723] R13: ffff92ec8431b400 R14: ffff92ec8431b508 R15: ffff92ec8fccec00
> [  113.496108] FS:  00007f5be5638840(0000) GS:ffff92f0ebb80000(0000) knlGS:0000000000000000
> [  113.496581] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  113.496907] CR2: 00007f5be54694b0 CR3: 0000000108e54003 CR4: 0000000000770ef0
> [  113.497308] PKRU: 55555554
> [  113.497469] Call Trace:
> [  113.497613]  <TASK>
> [  113.497741]  ? show_trace_log_lvl+0x1c4/0x2df
> [  113.497997]  ? show_trace_log_lvl+0x1c4/0x2df
> [  113.498251]  ? dm_put_table_device+0x64/0xd0 [dm_mod]
> [  113.498553]  ? __die_body.cold+0x8/0xd
> [  113.498768]  ? die+0x2b/0x50
> [  113.498937]  ? do_trap+0xce/0x120
> [  113.499129]  ? __fput_sync+0x25/0x30
> [  113.499337]  ? do_error_trap+0x65/0x80
> [  113.499577]  ? __fput_sync+0x25/0x30
> [  113.499787]  ? exc_invalid_op+0x4e/0x70
> [  113.500011]  ? __fput_sync+0x25/0x30
> [  113.500239]  ? asm_exc_invalid_op+0x16/0x20
> [  113.500842]  ? __fput_sync+0x25/0x30
> [  113.501387]  dm_put_table_device+0x64/0xd0 [dm_mod]
> [  113.502047]  dm_put_device+0x80/0x110 [dm_mod]
> [  113.502650]  stripe_dtr+0x2f/0x50 [dm_mod]
> [  113.503218]  dm_table_destroy+0x59/0x120 [dm_mod]
> [  113.503842]  __dm_destroy+0x114/0x1e0 [dm_mod]
> [  113.504402]  dm_hash_remove_all+0x63/0x160 [dm_mod]
> [  113.505028]  remove_all+0x1e/0x30 [dm_mod]
> [  113.505602]  ctl_ioctl+0x19f/0x290 [dm_mod]
> [  113.506146]  dm_ctl_ioctl+0xa/0x20 [dm_mod]
> [  113.506717]  __x64_sys_ioctl+0x87/0xc0
> [  113.507230]  do_syscall_64+0x5c/0xf0
> [  113.507755]  ? exc_page_fault+0x62/0x150
> [  113.508309]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
> [  113.508945] RIP: 0033:0x7f5be543ec6b
> 
> 
> 
> Thanks. 
> Ming
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR