[PATCH] Flush deferred queue when dm_suspend() is interrupted

[PATCH] Flush deferred queue when dm_suspend() is interrupted

[Jun'ichi Nomura]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

Hello,

I found in a code review that dm_suspend() doesn't flush
deferred queue when it's interrupted and failed to suspend.

Attached patch theoretically fixes this problem.
But I don't have a testcase to pick this point
and would like to hear comment from others whether
there is something to prevent this.

Also, modifying DMF_BLOCK_IO outside of io_lock may
cause a race like this:
   CPU0 (dm_suspend)         CPU1 (dm_request)
   -------------------------------------------------
   set_bit(DMF_BLOCK_IO)
   ...
   up_write(io_lock)
                               down_read(io_lock)
                               test_bit(DMF_BLOCK_IO)
                               up_read(io_lock)
                               queue_io(deferred)
                                 down_write(io_lock)
                                 test_bit(DMF_BLOCK_IO)
                                 up_write(io_lock)
   clear_bit(DMF_BLOCK_IO)

   The deferred I/O never be flushed until dm_resume().


For the failed dm_suspend() case, there is another problem
that presuspend is not reverted.
It may require a new method like suspend_cancel() for
any target which implements presuspend().

I would appreicate a comment for this as well.

Thanks,
-- 
Jun'ichi Nomura, NEC Solutions (America), Inc.

[-- Attachment #2: dm-flush-queue-eintr.patch --]
[-- Type: text/x-patch, Size: 727 bytes --]

When dm_suspend() is interrupted, it's possible that some bios are
queued to deferred list.
DMF_BLOCK_IO modification must be protected by io->lock.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>


--- linux-2.6.16-rc5.orig/drivers/md/dm.c	2006-02-27 00:09:35.000000000 -0500
+++ linux-2.6.16-rc5/drivers/md/dm.c	2006-03-08 14:54:09.000000000 -0500
@@ -1152,9 +1152,10 @@ int dm_suspend(struct mapped_device *md,
 	/* were we interrupted ? */
 	r = -EINTR;
 	if (atomic_read(&md->pending)) {
+		clear_bit(DMF_BLOCK_IO, &md->flags);
+		__flush_deferred_io(md, bio_list_get(&md->deferred));
 		up_write(&md->io_lock);
 		unlock_fs(md);
-		clear_bit(DMF_BLOCK_IO, &md->flags);
 		goto out;
 	}
 	up_write(&md->io_lock);

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Flush deferred queue when dm_suspend() is interrupted

[Alasdair G Kergon]

On Thu, Mar 09, 2006 at 06:48:29PM -0500, Jun'ichi Nomura wrote:
> I found in a code review that dm_suspend() doesn't flush
> deferred queue when it's interrupted and failed to suspend.
 
Yup - adding to tree.

> But I don't have a testcase to pick this point

Suspend the device below & issue I/O so the loop won't exit
until you abort the suspend.  That's the reason for that code.

> For the failed dm_suspend() case, there is another problem
> that presuspend is not reverted.

Not a problem yet: no user of that should do anything that needs
reverting should suspension fail, as per the comment (not a FIXME):
       /* This does not get reverted if there's an error later. */

If new targets or enhancements need new hooks then of course
we can consider adding them when the need arises.

Alasdair
-- 
agk@redhat.com