loop: Flush possible running bios when loop device is released.
PATCH 1/1 loop: Flush possible running bios when loop device is released.
Flush possible running bios when loop device is released.
When there are still queued bios and reference count drops to zero, loop device must flush all queued bios.
Otherwise it can lead to situation that caller closes the device, but some bios are still running and endio() function call later oopses when uses unallocated mempool.
This happens for example when running dm-crypt over loop, here is typical oops backtrace:
Oops: 0000 [#1] PREEMPT SMP EIP is at mempool_free+0x12/0x6b ... crypt_dec_pending+0x50/0x54 [dm_crypt] crypt_endio+0x9f/0xa7 [dm_crypt] crypt_endio+0x0/0xa7 [dm_crypt] bio_endio+0x2b/0x2e loop_thread+0x37a/0x3b1 do_lo_send_aops+0x0/0x165 autoremove_wake_function+0x0/0x33 loop_thread+0x0/0x3b1 kthread+0x3b/0x61 kthread+0x0/0x61 kernel_thread_helper+0x7/0x10
(But crash is reproducible with different dm targets running over loop device too.)
Patch fixes it by flushing the bios in release call, reusing the flush mechanism for switching backing store.
--- drivers/block/loop.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-)
drivers/block/loop.c +16 -1
@@ -624,20 +624,33 @@ static int loop_switch(struct loop_device *lo, struct file *file) }/* +* Helper to flush the IOs in loop, but keeping loop thread running +*/ +static int loop_flush (struct loop_device *lo) +{ +return loop_switch (lo,NULL ); +} + +/* * Do the actual switch; called from the BIO completion routine */ static void do_loop_switch (struct loop_device *lo,struct switch_request *p) {struct file *file = p->file;struct file *old_file = lo->lo_backing_file; -struct address_space *mapping = file->f_mapping; +struct address_space *mapping; + +if (!file) + goto out; + mapping = file->f_mapping;mapping_set_gfp_mask (old_file->f_mapping, lo->old_gfp_mask); lo->lo_backing_file = file; lo->lo_blocksize =S_ISBLK (mapping->host->i_mode) ? mapping->host->i_bdev->bd_block_size :PAGE_SIZE ; lo->old_gfp_mask =mapping_gfp_mask (mapping);mapping_set_gfp_mask (mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS)); +out:complete (&p->wait); }
@@ -1347,6 +1360,8 @@ static int lo_release(struct gendisk *disk, fmode_t mode)if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) && !lo->lo_refcnt)loop_clr_fd (lo,NULL ); +else if (!lo->lo_refcnt && lo->lo_thread) +loop_flush (lo);mutex_unlock (&lo->lo_ctl_mutex);
Quoted 53 lines · On Thu, 11 Dec 2008 21:37:02 +0100 Milan Broz <[email protected]> wrote:
Flush possible running bios when loop device is released.
When there are still queued bios and reference count drops to zero, loop device must flush all queued bios.
Otherwise it can lead to situation that caller closes the device, but some bios are still running and
endio()function call later oopses when uses unallocated mempool.This happens for example when running dm-crypt over loop, here is typical oops backtrace:
Oops: 0000 [#1] PREEMPT SMP EIP is at mempool_free+0x12/0x6b ... crypt_dec_pending+0x50/0x54 [dm_crypt] crypt_endio+0x9f/0xa7 [dm_crypt] crypt_endio+0x0/0xa7 [dm_crypt] bio_endio+0x2b/0x2e loop_thread+0x37a/0x3b1 do_lo_send_aops+0x0/0x165 autoremove_wake_function+0x0/0x33 loop_thread+0x0/0x3b1 kthread+0x3b/0x61 kthread+0x0/0x61 kernel_thread_helper+0x7/0x10(But crash is reproducible with different dm targets running over loop device too.)
Patch fixes it by flushing the bios in release call, reusing the flush mechanism for switching backing store.
drivers/block/loop.c +8
-0@@ -624,20 +624,33 @@ static int loop_switch(struct loop_device *lo, struct file *file) }/* +* Helper to flush the IOs in loop, but keeping loop thread running +*/ +static int loop_flush (struct loop_device *lo) +{ +return loop_switch (lo,NULL ); +} + +/* * Do the actual switch; called from the BIO completion routine
drivers/block/loop.c static int loop_switch(struct loop_device *lo, struct file *file) · line 644
Quoted 10 lines
drivers/block/loop.c +4
-1*/ static void do_loop_switch (struct loop_device *lo,struct switch_request *p) {struct file *file = p->file;struct file *old_file = lo->lo_backing_file; -struct address_space *mapping = file->f_mapping; +struct address_space *mapping; + +if (!file) + goto out;
Why are we doing this here?
(whatever the reason, others will wonder the same thing. A comment will fix that bug).
Quoted 12 lines
drivers/block/loop.c +2
-0+ mapping = file->f_mapping;mapping_set_gfp_mask (old_file->f_mapping, lo->old_gfp_mask); lo->lo_backing_file = file; lo->lo_blocksize =S_ISBLK (mapping->host->i_mode) ? mapping->host->i_bdev->bd_block_size :PAGE_SIZE ; lo->old_gfp_mask =mapping_gfp_mask (mapping);mapping_set_gfp_mask (mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS)); +out:complete (&p->wait); }
drivers/block/loop.c static int lo_release(struct gendisk *disk, fmode_t mode) · line 1365
Quoted 6 lines
drivers/block/loop.c +2
-0@@ -1347,6 +1360,8 @@ static int lo_release(struct gendisk *disk, fmode_t mode)if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) && !lo->lo_refcnt)loop_clr_fd (lo,NULL ); +else if (!lo->lo_refcnt && lo->lo_thread) +loop_flush (lo);
Why does the code test for null ->lo_thread here? afaict that can only get cleared in loop_clr_fd(), and we didn't call that?
Also, would it not be clearer to do
if (!lo->lo_refcnt) {
if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
/*
* comment explaining what's going on goes here
*/
loop_clr_fd(lo, NULL);
} else if (lo->lo_thread) {
/*
* ditto
*/
loop_flush(lo);
}
}?
Quoted 79 lines · Milan Broz
Flush possible running bios when loop device is released.
When there are still queued bios and reference count drops to zero, loop device must flush all queued bios.
Otherwise it can lead to situation that caller closes the device, but some bios are still running and
endio()function call later oopses when uses unallocated mempool.This happens for example when running dm-crypt over loop, here is typical oops backtrace:
Oops: 0000 [#1] PREEMPT SMP EIP is at mempool_free+0x12/0x6b ... crypt_dec_pending+0x50/0x54 [dm_crypt] crypt_endio+0x9f/0xa7 [dm_crypt] crypt_endio+0x0/0xa7 [dm_crypt] bio_endio+0x2b/0x2e loop_thread+0x37a/0x3b1 do_lo_send_aops+0x0/0x165 autoremove_wake_function+0x0/0x33 loop_thread+0x0/0x3b1 kthread+0x3b/0x61 kthread+0x0/0x61 kernel_thread_helper+0x7/0x10(But crash is reproducible with different dm targets running over loop device too.)
Patch fixes it by flushing the bios in release call, reusing the flush mechanism for switching backing store.
Signed-off-by Milan Broz <[email protected]>--- drivers/block/loop.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-)drivers/block/loop.c +14
-1@@ -624,20 +624,33 @@ static int loop_switch(struct loop_device *lo, struct file *file) }/* +* Helper to flush the IOs in loop, but keeping loop thread running +*/ +static int loop_flush (struct loop_device *lo) +{ +return loop_switch (lo,NULL ); +} + +/* * Do the actual switch; called from the BIO completion routine */ static void do_loop_switch (struct loop_device *lo,struct switch_request *p) {struct file *file = p->file;struct file *old_file = lo->lo_backing_file; -struct address_space *mapping = file->f_mapping; +struct address_space *mapping; + +if (!file) + goto out; + mapping = file->f_mapping;mapping_set_gfp_mask (old_file->f_mapping, lo->old_gfp_mask); lo->lo_backing_file = file; lo->lo_blocksize =S_ISBLK (mapping->host->i_mode) ? mapping->host->i_bdev->bd_block_size :PAGE_SIZE ; lo->old_gfp_mask =mapping_gfp_mask (mapping);mapping_set_gfp_mask (mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS)); +out:complete (&p->wait); }
drivers/block/loop.c static int lo_release(struct gendisk *disk, fmode_t mode) · line 1366
Quoted 7 lines
drivers/block/loop.c +2
-0@@ -1347,6 +1360,8 @@ static int lo_release(struct gendisk *disk, fmode_t mode)if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) && !lo->lo_refcnt)loop_clr_fd (lo,NULL ); +else if (!lo->lo_refcnt && lo->lo_thread) +loop_flush (lo);mutex_unlock (&lo->lo_ctl_mutex);
Looks very good to me. I have nothing further to add except what Andrew already detailed. Care to rediff with those changes and resend? Then I'll queue it up.
Signature · 1 line
Jens Axboe
PATCH v2 1/1 loop: Flush possible running bios when loop device is released.
Flush possible running bios when loop device is released.
When there are still queued bios and reference count drops to zero, loop device must flush all queued bios.
Otherwise it can lead to situation that caller closes the device, but some bios are still running and endio() function call later OOpses when uses unallocated mempool.
This happens for example when running dm-crypt over loop, here is typical oops backtrace:
Oops: 0000 [#1] PREEMPT SMP EIP is at mempool_free+0x12/0x6b ... crypt_dec_pending+0x50/0x54 [dm_crypt] crypt_endio+0x9f/0xa7 [dm_crypt] crypt_endio+0x0/0xa7 [dm_crypt] bio_endio+0x2b/0x2e loop_thread+0x37a/0x3b1 do_lo_send_aops+0x0/0x165 autoremove_wake_function+0x0/0x33 loop_thread+0x0/0x3b1 kthread+0x3b/0x61 kthread+0x0/0x61 kernel_thread_helper+0x7/0x10
(But crash is reproducible with different dm targets running over loop device too.)
Patch fixes it by flushing the bios in release call, reusing the flush mechanism for switching backing store.
--- drivers/block/loop.c | 37 ++++++++++++++++++++++++++++++++++--- 1 files changed, 34 insertions(+), 3 deletions(-)
drivers/block/loop.c +34 -3
@@ -624,20 +624,38 @@ static int loop_switch(struct loop_device *lo, struct file *file) }/* +* Helper to flush the IOs in loop, but keeping loop thread running +*/ +static int loop_flush (struct loop_device *lo) +{ +/* loop not yet configured, no running thread, nothing to flush */ +if (!lo->lo_thread) +return 0 ; + +return loop_switch (lo,NULL ); +} + +/* * Do the actual switch; called from the BIO completion routine */ static void do_loop_switch (struct loop_device *lo,struct switch_request *p) {struct file *file = p->file;struct file *old_file = lo->lo_backing_file; -struct address_space *mapping = file->f_mapping; +struct address_space *mapping; + +/* if no new file, only flush of queued bios requested */ +if (!file) + goto out; + mapping = file->f_mapping;mapping_set_gfp_mask (old_file->f_mapping, lo->old_gfp_mask); lo->lo_backing_file = file; lo->lo_blocksize =S_ISBLK (mapping->host->i_mode) ? mapping->host->i_bdev->bd_block_size :PAGE_SIZE ; lo->old_gfp_mask =mapping_gfp_mask (mapping);mapping_set_gfp_mask (mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS)); +out:complete (&p->wait); }
@@ -1343,11 +1361,24 @@ static int lo_release(struct gendisk *disk, fmode_t mode)struct loop_device *lo = disk->private_data;mutex_lock (&lo->lo_ctl_mutex); - --lo->lo_refcnt; -if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) && !lo->lo_refcnt) +if (--lo->lo_refcnt) + goto out; + +if (lo->lo_flags &LO_FLAGS_AUTOCLEAR ) +/* +* In autoclear mode, stop the loop thread +* and remove configuration after last close. +*/ loop_clr_fd (lo,NULL ); +else +/* +* Otherwise keep thread (if running) and config, +* but flush possible ongoing bios in thread. +*/ +loop_flush (lo); +out:mutex_unlock (&lo->lo_ctl_mutex);return 0 ;
Quoted 100 lines · Milan Broz
Flush possible running bios when loop device is released.
When there are still queued bios and reference count drops to zero, loop device must flush all queued bios.
Otherwise it can lead to situation that caller closes the device, but some bios are still running and
endio()function call later OOpses when uses unallocated mempool.This happens for example when running dm-crypt over loop, here is typical oops backtrace:
Oops: 0000 [#1] PREEMPT SMP EIP is at mempool_free+0x12/0x6b ... crypt_dec_pending+0x50/0x54 [dm_crypt] crypt_endio+0x9f/0xa7 [dm_crypt] crypt_endio+0x0/0xa7 [dm_crypt] bio_endio+0x2b/0x2e loop_thread+0x37a/0x3b1 do_lo_send_aops+0x0/0x165 autoremove_wake_function+0x0/0x33 loop_thread+0x0/0x3b1 kthread+0x3b/0x61 kthread+0x0/0x61 kernel_thread_helper+0x7/0x10(But crash is reproducible with different dm targets running over loop device too.)
Patch fixes it by flushing the bios in release call, reusing the flush mechanism for switching backing store.
Signed-off-by Milan Broz <[email protected]>--- drivers/block/loop.c | 37 ++++++++++++++++++++++++++++++++++--- 1 files changed, 34 insertions(+), 3 deletions(-)drivers/block/loop.c +28
-3@@ -624,20 +624,38 @@ static int loop_switch(struct loop_device *lo, struct file *file) }/* +* Helper to flush the IOs in loop, but keeping loop thread running +*/ +static int loop_flush (struct loop_device *lo) +{ +/* loop not yet configured, no running thread, nothing to flush */ +if (!lo->lo_thread) +return 0 ; + +return loop_switch (lo,NULL ); +} + +/* * Do the actual switch; called from the BIO completion routine */ static void do_loop_switch (struct loop_device *lo,struct switch_request *p) {struct file *file = p->file;struct file *old_file = lo->lo_backing_file; -struct address_space *mapping = file->f_mapping; +struct address_space *mapping; + +/* if no new file, only flush of queued bios requested */ +if (!file) + goto out; + mapping = file->f_mapping;mapping_set_gfp_mask (old_file->f_mapping, lo->old_gfp_mask); lo->lo_backing_file = file; lo->lo_blocksize =S_ISBLK (mapping->host->i_mode) ? mapping->host->i_bdev->bd_block_size :PAGE_SIZE ; lo->old_gfp_mask =mapping_gfp_mask (mapping);mapping_set_gfp_mask (mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS)); +out:complete (&p->wait); }@@ -1343,11 +1361,24 @@ static int lo_release(struct gendisk *disk, fmode_t mode)struct loop_device *lo = disk->private_data;mutex_lock (&lo->lo_ctl_mutex); - --lo->lo_refcnt; -if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) && !lo->lo_refcnt) +if (--lo->lo_refcnt) + goto out; + +if (lo->lo_flags &LO_FLAGS_AUTOCLEAR ) +/* +* In autoclear mode, stop the loop thread +* and remove configuration after last close. +*/ loop_clr_fd (lo,NULL ); +else
drivers/block/loop.c static int lo_release(struct gendisk *disk, fmode_t mode) · line 1384
Quoted 10 lines
drivers/block/loop.c +6
-0+/* +* Otherwise keep thread (if running) and config, +* but flush possible ongoing bios in thread. +*/ +loop_flush (lo); +out:mutex_unlock (&lo->lo_ctl_mutex);return 0 ;
That looks good, thanks. I do prefer braces when using comments like that, I'll add them while merging...
Signature · 1 line
Jens Axboe