[Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Michael S. Tsirkin]

Summary: seem to need to revert 213d9417fec62ef4c3675621b9364a667954d4dd
to fix resume from hibernation. Bugzilla entry created:
http://bugzilla.kernel.org/show_bug.cgi?id=12713

On kernel 2.6.29-rc3 and earlier, suspend/resume from disk
(hibernation) works fine on my T60p with dm crypt full disk encryption.

Starting with 2.6.29-rc4, I get hangs on resume from suspend to disk.
On resume, after a bit of regular activity, the disk LED lights up
with an unusual dull color and stays that way.
Pressing CTRL-ALT-DEL causes linux to print a message that it will
reboot, and reboot after a couple of seconds.

I bisected and got to the following commit:
commit 213d9417fec62ef4c3675621b9364a667954d4dd
Author: Jens Axboe <jens.axboe@oracle.com>
Date:   Tue Jan 6 09:16:05 2009 +0100

    block: seperate bio/request unplug and sync bits

    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

I reverted the commit 213d9417fec62ef4c3675621b9364a667954d4dd
on top of 2.6.29-rc5 (resolving a trivial conflict) and verified
that reverting this commit fixes hibernate for me.

Bisect log below:

git bisect start
# bad: [8e4921515c1a379539607eb443d51c30f4f7f338] Linux 2.6.29-rc4
git bisect bad 8e4921515c1a379539607eb443d51c30f4f7f338
# good: [18e352e4a73465349711a9324767e1b2453383e2] Linux 2.6.29-rc3
git bisect good 18e352e4a73465349711a9324767e1b2453383e2
# bad: [5c350d93ff4736086a1b08fef1d0b5e22138d2e0] Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/drzeus/mmc
git bisect bad 5c350d93ff4736086a1b08fef1d0b5e22138d2e0
# good: [bb9f113f5ca7d182256dee69bcaebd4c79062305] headers_check fix:
sound/hdsp.h
git bisect good bb9f113f5ca7d182256dee69bcaebd4c79062305
# bad: [2d2eca4d11933bd37a4944aae06e6122efffaea8] MIPS: Alchemy: time.c build
fix
git bisect bad 2d2eca4d11933bd37a4944aae06e6122efffaea8
# bad: [ae704e9f92f87b12c5938b07245792857c7c9c14] Merge branch 'for-linus' of
git://git.kernel.dk/linux-2.6-block
git bisect bad ae704e9f92f87b12c5938b07245792857c7c9c14
# good: [584dbe9475313e117abf9d2af88164edfd429c9a] netxen: fix memory leak in
drivers/net/netxen_nic_init.c
git bisect good 584dbe9475313e117abf9d2af88164edfd429c9a
# bad: [7598909e3ee2a08726276d6415b69dadb52d0d76] Mark mandatory elevator
functions in the biodoc.txt
git bisect bad 7598909e3ee2a08726276d6415b69dadb52d0d76
# good: [1308835ffffe6d61ad1f48c5c381c9cc47f683ec] block: export
SSD/non-rotational queue flag through sysfs
git bisect good 1308835ffffe6d61ad1f48c5c381c9cc47f683ec
# bad: [dbdac9b71dff5d27885f82eb2cfca310861fdf9e] block: Fix documentation for
blkdev_issue_flush()
git bisect bad dbdac9b71dff5d27885f82eb2cfca310861fdf9e
# bad: [1dfa17f4ab8543d82caf4d36636b93916a18f456] block: add bio_rw_flagged()
for testing bio->bi_rw
# Had trouble booting, booted second time after reset
git bisect bad 1dfa17f4ab8543d82caf4d36636b93916a18f456
# bad: [213d9417fec62ef4c3675621b9364a667954d4dd] block: seperate bio/request
unplug and sync bits
# Shows suspend progress nicely, some other commits tested don't
git bisect bad 213d9417fec62ef4c3675621b9364a667954d4dd


.config attached to bugzilla entry

Re: [Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Michael S. Tsirkin]

On Mon, Feb 16, 2009 at 2:05 AM, Michael S. Tsirkin
<m.s.tsirkin@gmail.com> wrote:
> Summary: seem to need to revert 213d9417fec62ef4c3675621b9364a667954d4dd
> to fix resume from hibernation. Bugzilla entry created:
> http://bugzilla.kernel.org/show_bug.cgi?id=12713

Looking over this, I see something strange in the commit in question:

diff --git a/include/linux/bio.h b/include/linux/bio.h
index 5175aa3..f53568c 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -163,12 +163,15 @@ struct bio {
 #define BIO_RW         0       /* Must match RW in req flags (blkdev.h) */
 #define BIO_RW_AHEAD   1       /* Must match FAILFAST in req flags */
 #define BIO_RW_BARRIER 2
-#define BIO_RW_SYNC    3
-#define BIO_RW_META    4
-#define BIO_RW_DISCARD 5
-#define BIO_RW_FAILFAST_DEV            6
-#define BIO_RW_FAILFAST_TRANSPORT      7
-#define BIO_RW_FAILFAST_DRIVER         8
+#define BIO_RW_SYNCIO  3
+#define BIO_RW_UNPLUG  4
+#define BIO_RW_META    5
+#define BIO_RW_DISCARD 6
+#define BIO_RW_FAILFAST_DEV            7
+#define BIO_RW_FAILFAST_TRANSPORT      8
+#define BIO_RW_FAILFAST_DRIVER         9
+
+#define BIO_RW_SYNC    (BIO_RW_SYNCIO | BIO_RW_UNPLUG)

 /*
  * upper 16 bits of bi_rw define the io priority of this bio

I haven't read the code in depth, but taking running numbers and doing
bitwise "or"
on them looks a bit strange to me.
So here BIO_RW_SYNC is (3 | 4) = 7, that is the same as BIO_RW_FAILFAST_DEV.
So for example bio_failfast_dev and bio_sync are the same.

Jens, could you comment on this please? Is this intentional?

-- 
MST

Re: [Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Roland Dreier]

 > I haven't read the code in depth, but taking running numbers and doing
 > bitwise "or"
 > on them looks a bit strange to me.
 > So here BIO_RW_SYNC is (3 | 4) = 7, that is the same as BIO_RW_FAILFAST_DEV.
 > So for example bio_failfast_dev and bio_sync are the same.

Yes, this is clearly wrong.  The fix seems to be to delete the
definition of BIO_RW_SYNC, and everywhere that breaks, replace

	1 << BIO_RW_SYNC

with

	(1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG)

 - R.

Re: [Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Jens Axboe]

On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
> On Mon, Feb 16, 2009 at 2:05 AM, Michael S. Tsirkin
> <m.s.tsirkin@gmail.com> wrote:
> > Summary: seem to need to revert 213d9417fec62ef4c3675621b9364a667954d4dd
> > to fix resume from hibernation. Bugzilla entry created:
> > http://bugzilla.kernel.org/show_bug.cgi?id=12713
> 
> Looking over this, I see something strange in the commit in question:
> 
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 5175aa3..f53568c 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -163,12 +163,15 @@ struct bio {
>  #define BIO_RW         0       /* Must match RW in req flags (blkdev.h) */
>  #define BIO_RW_AHEAD   1       /* Must match FAILFAST in req flags */
>  #define BIO_RW_BARRIER 2
> -#define BIO_RW_SYNC    3
> -#define BIO_RW_META    4
> -#define BIO_RW_DISCARD 5
> -#define BIO_RW_FAILFAST_DEV            6
> -#define BIO_RW_FAILFAST_TRANSPORT      7
> -#define BIO_RW_FAILFAST_DRIVER         8
> +#define BIO_RW_SYNCIO  3
> +#define BIO_RW_UNPLUG  4
> +#define BIO_RW_META    5
> +#define BIO_RW_DISCARD 6
> +#define BIO_RW_FAILFAST_DEV            7
> +#define BIO_RW_FAILFAST_TRANSPORT      8
> +#define BIO_RW_FAILFAST_DRIVER         9
> +
> +#define BIO_RW_SYNC    (BIO_RW_SYNCIO | BIO_RW_UNPLUG)
> 
>  /*
>   * upper 16 bits of bi_rw define the io priority of this bio
> 
> I haven't read the code in depth, but taking running numbers and doing
> bitwise "or"
> on them looks a bit strange to me.
> So here BIO_RW_SYNC is (3 | 4) = 7, that is the same as BIO_RW_FAILFAST_DEV.
> So for example bio_failfast_dev and bio_sync are the same.
> 
> Jens, could you comment on this please? Is this intentional?

That's clearly a braino, you can't OR the shift values of course. I'll
get it fixed up asap!

-- 
Jens Axboe

Re: [Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Michael S. Tsirkin]

On Mon, Feb 16, 2009 at 9:27 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
>> On Mon, Feb 16, 2009 at 2:05 AM, Michael S. Tsirkin
>> <m.s.tsirkin@gmail.com> wrote:
>> > Summary: seem to need to revert 213d9417fec62ef4c3675621b9364a667954d4dd
>> > to fix resume from hibernation. Bugzilla entry created:
>> > http://bugzilla.kernel.org/show_bug.cgi?id=12713
>>
>> Looking over this, I see something strange in the commit in question:
>>
>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>> index 5175aa3..f53568c 100644
>> --- a/include/linux/bio.h
>> +++ b/include/linux/bio.h
>> @@ -163,12 +163,15 @@ struct bio {
>>  #define BIO_RW         0       /* Must match RW in req flags (blkdev.h) */
>>  #define BIO_RW_AHEAD   1       /* Must match FAILFAST in req flags */
>>  #define BIO_RW_BARRIER 2
>> -#define BIO_RW_SYNC    3
>> -#define BIO_RW_META    4
>> -#define BIO_RW_DISCARD 5
>> -#define BIO_RW_FAILFAST_DEV            6
>> -#define BIO_RW_FAILFAST_TRANSPORT      7
>> -#define BIO_RW_FAILFAST_DRIVER         8
>> +#define BIO_RW_SYNCIO  3
>> +#define BIO_RW_UNPLUG  4
>> +#define BIO_RW_META    5
>> +#define BIO_RW_DISCARD 6
>> +#define BIO_RW_FAILFAST_DEV            7
>> +#define BIO_RW_FAILFAST_TRANSPORT      8
>> +#define BIO_RW_FAILFAST_DRIVER         9
>> +
>> +#define BIO_RW_SYNC    (BIO_RW_SYNCIO | BIO_RW_UNPLUG)
>>
>>  /*
>>   * upper 16 bits of bi_rw define the io priority of this bio
>>
>> I haven't read the code in depth, but taking running numbers and doing
>> bitwise "or"
>> on them looks a bit strange to me.
>> So here BIO_RW_SYNC is (3 | 4) = 7, that is the same as BIO_RW_FAILFAST_DEV.
>> So for example bio_failfast_dev and bio_sync are the same.
>>
>> Jens, could you comment on this please? Is this intentional?
>
> That's clearly a braino, you can't OR the shift values of course. I'll
> get it fixed up asap!

Cool, send me a patch, I'll test.

> --
> Jens Axboe
>
>

Re: [Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Jens Axboe]

On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
> On Mon, Feb 16, 2009 at 9:27 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
> >> On Mon, Feb 16, 2009 at 2:05 AM, Michael S. Tsirkin
> >> <m.s.tsirkin@gmail.com> wrote:
> >> > Summary: seem to need to revert 213d9417fec62ef4c3675621b9364a667954d4dd
> >> > to fix resume from hibernation. Bugzilla entry created:
> >> > http://bugzilla.kernel.org/show_bug.cgi?id=12713
> >>
> >> Looking over this, I see something strange in the commit in question:
> >>
> >> diff --git a/include/linux/bio.h b/include/linux/bio.h
> >> index 5175aa3..f53568c 100644
> >> --- a/include/linux/bio.h
> >> +++ b/include/linux/bio.h
> >> @@ -163,12 +163,15 @@ struct bio {
> >>  #define BIO_RW         0       /* Must match RW in req flags (blkdev.h) */
> >>  #define BIO_RW_AHEAD   1       /* Must match FAILFAST in req flags */
> >>  #define BIO_RW_BARRIER 2
> >> -#define BIO_RW_SYNC    3
> >> -#define BIO_RW_META    4
> >> -#define BIO_RW_DISCARD 5
> >> -#define BIO_RW_FAILFAST_DEV            6
> >> -#define BIO_RW_FAILFAST_TRANSPORT      7
> >> -#define BIO_RW_FAILFAST_DRIVER         8
> >> +#define BIO_RW_SYNCIO  3
> >> +#define BIO_RW_UNPLUG  4
> >> +#define BIO_RW_META    5
> >> +#define BIO_RW_DISCARD 6
> >> +#define BIO_RW_FAILFAST_DEV            7
> >> +#define BIO_RW_FAILFAST_TRANSPORT      8
> >> +#define BIO_RW_FAILFAST_DRIVER         9
> >> +
> >> +#define BIO_RW_SYNC    (BIO_RW_SYNCIO | BIO_RW_UNPLUG)
> >>
> >>  /*
> >>   * upper 16 bits of bi_rw define the io priority of this bio
> >>
> >> I haven't read the code in depth, but taking running numbers and doing
> >> bitwise "or"
> >> on them looks a bit strange to me.
> >> So here BIO_RW_SYNC is (3 | 4) = 7, that is the same as BIO_RW_FAILFAST_DEV.
> >> So for example bio_failfast_dev and bio_sync are the same.
> >>
> >> Jens, could you comment on this please? Is this intentional?
> >
> > That's clearly a braino, you can't OR the shift values of course. I'll
> > get it fixed up asap!
> 
> Cool, send me a patch, I'll test.

Try this. Not a huge fan of this approach, but it should bring behaviour
back to what it used to be.

diff --git a/block/blktrace.c b/block/blktrace.c
index 39cc3bf..7cf9d1f 100644
--- a/block/blktrace.c
+++ b/block/blktrace.c
@@ -142,7 +142,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 
 	what |= ddir_act[rw & WRITE];
 	what |= MASK_TC_BIT(rw, BARRIER);
-	what |= MASK_TC_BIT(rw, SYNC);
+	what |= MASK_TC_BIT(rw, SYNCIO);
 	what |= MASK_TC_BIT(rw, AHEAD);
 	what |= MASK_TC_BIT(rw, META);
 	what |= MASK_TC_BIT(rw, DISCARD);
diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
index a343385..f14813b 100644
--- a/drivers/md/dm-io.c
+++ b/drivers/md/dm-io.c
@@ -328,7 +328,7 @@ static void dispatch_io(int rw, unsigned int num_regions,
 	struct dpages old_pages = *dp;
 
 	if (sync)
-		rw |= (1 << BIO_RW_SYNC);
+		rw |= (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
 
 	/*
 	 * For multiple regions we need to be careful to rewind
diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c
index 3073618..0a225da 100644
--- a/drivers/md/dm-kcopyd.c
+++ b/drivers/md/dm-kcopyd.c
@@ -344,7 +344,7 @@ static int run_io_job(struct kcopyd_job *job)
 {
 	int r;
 	struct dm_io_request io_req = {
-		.bi_rw = job->rw | (1 << BIO_RW_SYNC),
+		.bi_rw = job->rw | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG),
 		.mem.type = DM_IO_PAGE_LIST,
 		.mem.ptr.pl = job->pages,
 		.mem.offset = job->offset,
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 4495104..03b4cd0 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -474,7 +474,7 @@ void md_super_write(mddev_t *mddev, mdk_rdev_t *rdev,
 	 * causes ENOTSUPP, we allocate a spare bio...
 	 */
 	struct bio *bio = bio_alloc(GFP_NOIO, 1);
-	int rw = (1<<BIO_RW) | (1<<BIO_RW_SYNC);
+	int rw = (1<<BIO_RW) | (1<<BIO_RW_SYNCIO) | (1<<BIO_RW_UNPLUG);
 
 	bio->bi_bdev = rdev->bdev;
 	bio->bi_sector = sector;
@@ -531,7 +531,7 @@ int sync_page_io(struct block_device *bdev, sector_t sector, int size,
 	struct completion event;
 	int ret;
 
-	rw |= (1 << BIO_RW_SYNC);
+	rw |= (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
 
 	bio->bi_bdev = bdev;
 	bio->bi_sector = sector;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 2aa283a..1b16108 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -171,8 +171,6 @@ struct bio {
 #define BIO_RW_FAILFAST_TRANSPORT	8
 #define BIO_RW_FAILFAST_DRIVER		9
 
-#define BIO_RW_SYNC	(BIO_RW_SYNCIO | BIO_RW_UNPLUG)
-
 #define bio_rw_flagged(bio, flag)	((bio)->bi_rw & (1 << (flag)))
 
 /*
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index 25379cb..6e91587 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -15,6 +15,7 @@ enum blktrace_cat {
 	BLK_TC_WRITE	= 1 << 1,	/* writes */
 	BLK_TC_BARRIER	= 1 << 2,	/* barrier */
 	BLK_TC_SYNC	= 1 << 3,	/* sync IO */
+	BLK_TC_SYNCIO	= BLK_TC_SYNC,
 	BLK_TC_QUEUE	= 1 << 4,	/* queueing/merging */
 	BLK_TC_REQUEUE	= 1 << 5,	/* requeueing */
 	BLK_TC_ISSUE	= 1 << 6,	/* issue */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6022f44..67857dc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -87,10 +87,10 @@ struct inodes_stat_t {
 #define WRITE 1
 #define READA 2		/* read-ahead  - don't block if no resources */
 #define SWRITE 3	/* for ll_rw_block() - wait for buffer lock */
-#define READ_SYNC	(READ | (1 << BIO_RW_SYNC))
+#define READ_SYNC	(READ | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG))
 #define READ_META	(READ | (1 << BIO_RW_META))
-#define WRITE_SYNC	(WRITE | (1 << BIO_RW_SYNC))
-#define SWRITE_SYNC	(SWRITE | (1 << BIO_RW_SYNC))
+#define WRITE_SYNC	(WRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG))
+#define SWRITE_SYNC	(SWRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG))
 #define WRITE_BARRIER	(WRITE | (1 << BIO_RW_BARRIER))
 #define DISCARD_NOBARRIER (1 << BIO_RW_DISCARD)
 #define DISCARD_BARRIER ((1 << BIO_RW_DISCARD) | (1 << BIO_RW_BARRIER))
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 6da1435..505f319 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -60,6 +60,7 @@ static struct block_device *resume_bdev;
 static int submit(int rw, pgoff_t page_off, struct page *page,
 			struct bio **bio_chain)
 {
+	const int bio_rw = rw | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
 	struct bio *bio;
 
 	bio = bio_alloc(__GFP_WAIT | __GFP_HIGH, 1);
@@ -80,7 +81,7 @@ static int submit(int rw, pgoff_t page_off, struct page *page,
 	bio_get(bio);
 
 	if (bio_chain == NULL) {
-		submit_bio(rw | (1 << BIO_RW_SYNC), bio);
+		submit_bio(bio_rw, bio);
 		wait_on_page_locked(page);
 		if (rw == READ)
 			bio_set_pages_dirty(bio);
@@ -90,7 +91,7 @@ static int submit(int rw, pgoff_t page_off, struct page *page,
 			get_page(page);	/* These pages are freed later */
 		bio->bi_private = *bio_chain;
 		*bio_chain = bio;
-		submit_bio(rw | (1 << BIO_RW_SYNC), bio);
+		submit_bio(bio_rw, bio);
 	}
 	return 0;
 }
diff --git a/mm/page_io.c b/mm/page_io.c
index dc6ce0a..3023c47 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -111,7 +111,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
 		goto out;
 	}
 	if (wbc->sync_mode == WB_SYNC_ALL)
-		rw |= (1 << BIO_RW_SYNC);
+		rw |= (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
 	count_vm_event(PSWPOUT);
 	set_page_writeback(page);
 	unlock_page(page);

-- 
Jens Axboe

Re: [Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Michael S. Tsirkin]

On Mon, Feb 16, 2009 at 09:47:32AM +0100, Jens Axboe wrote:
> On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
> > On Mon, Feb 16, 2009 at 9:27 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> > > On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
> > >> On Mon, Feb 16, 2009 at 2:05 AM, Michael S. Tsirkin
> > >> <m.s.tsirkin@gmail.com> wrote:
> > >> > Summary: seem to need to revert 213d9417fec62ef4c3675621b9364a667954d4dd
> > >> > to fix resume from hibernation. Bugzilla entry created:
> > >> > http://bugzilla.kernel.org/show_bug.cgi?id=12713
> > >>
> > >> Looking over this, I see something strange in the commit in question:
> > >>
> > >> diff --git a/include/linux/bio.h b/include/linux/bio.h
> > >> index 5175aa3..f53568c 100644
> > >> --- a/include/linux/bio.h
> > >> +++ b/include/linux/bio.h
> > >> @@ -163,12 +163,15 @@ struct bio {
> > >>  #define BIO_RW         0       /* Must match RW in req flags (blkdev.h) */
> > >>  #define BIO_RW_AHEAD   1       /* Must match FAILFAST in req flags */
> > >>  #define BIO_RW_BARRIER 2
> > >> -#define BIO_RW_SYNC    3
> > >> -#define BIO_RW_META    4
> > >> -#define BIO_RW_DISCARD 5
> > >> -#define BIO_RW_FAILFAST_DEV            6
> > >> -#define BIO_RW_FAILFAST_TRANSPORT      7
> > >> -#define BIO_RW_FAILFAST_DRIVER         8
> > >> +#define BIO_RW_SYNCIO  3
> > >> +#define BIO_RW_UNPLUG  4
> > >> +#define BIO_RW_META    5
> > >> +#define BIO_RW_DISCARD 6
> > >> +#define BIO_RW_FAILFAST_DEV            7
> > >> +#define BIO_RW_FAILFAST_TRANSPORT      8
> > >> +#define BIO_RW_FAILFAST_DRIVER         9
> > >> +
> > >> +#define BIO_RW_SYNC    (BIO_RW_SYNCIO | BIO_RW_UNPLUG)
> > >>
> > >>  /*
> > >>   * upper 16 bits of bi_rw define the io priority of this bio
> > >>
> > >> I haven't read the code in depth, but taking running numbers and doing
> > >> bitwise "or"
> > >> on them looks a bit strange to me.
> > >> So here BIO_RW_SYNC is (3 | 4) = 7, that is the same as BIO_RW_FAILFAST_DEV.
> > >> So for example bio_failfast_dev and bio_sync are the same.
> > >>
> > >> Jens, could you comment on this please? Is this intentional?
> > >
> > > That's clearly a braino, you can't OR the shift values of course. I'll
> > > get it fixed up asap!
> > 
> > Cool, send me a patch, I'll test.
> 
> Try this. Not a huge fan of this approach, but it should bring behaviour
> back to what it used to be.

I verified that applying this patch on top of 2.6.29-rc5 fixes
hibernation for me.

> 
> diff --git a/block/blktrace.c b/block/blktrace.c
> index 39cc3bf..7cf9d1f 100644
> --- a/block/blktrace.c
> +++ b/block/blktrace.c
> @@ -142,7 +142,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
>  
>  	what |= ddir_act[rw & WRITE];
>  	what |= MASK_TC_BIT(rw, BARRIER);
> -	what |= MASK_TC_BIT(rw, SYNC);
> +	what |= MASK_TC_BIT(rw, SYNCIO);
>  	what |= MASK_TC_BIT(rw, AHEAD);
>  	what |= MASK_TC_BIT(rw, META);
>  	what |= MASK_TC_BIT(rw, DISCARD);
> diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
> index a343385..f14813b 100644
> --- a/drivers/md/dm-io.c
> +++ b/drivers/md/dm-io.c
> @@ -328,7 +328,7 @@ static void dispatch_io(int rw, unsigned int num_regions,
>  	struct dpages old_pages = *dp;
>  
>  	if (sync)
> -		rw |= (1 << BIO_RW_SYNC);
> +		rw |= (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
>  
>  	/*
>  	 * For multiple regions we need to be careful to rewind
> diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c
> index 3073618..0a225da 100644
> --- a/drivers/md/dm-kcopyd.c
> +++ b/drivers/md/dm-kcopyd.c
> @@ -344,7 +344,7 @@ static int run_io_job(struct kcopyd_job *job)
>  {
>  	int r;
>  	struct dm_io_request io_req = {
> -		.bi_rw = job->rw | (1 << BIO_RW_SYNC),
> +		.bi_rw = job->rw | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG),
>  		.mem.type = DM_IO_PAGE_LIST,
>  		.mem.ptr.pl = job->pages,
>  		.mem.offset = job->offset,
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 4495104..03b4cd0 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -474,7 +474,7 @@ void md_super_write(mddev_t *mddev, mdk_rdev_t *rdev,
>  	 * causes ENOTSUPP, we allocate a spare bio...
>  	 */
>  	struct bio *bio = bio_alloc(GFP_NOIO, 1);
> -	int rw = (1<<BIO_RW) | (1<<BIO_RW_SYNC);
> +	int rw = (1<<BIO_RW) | (1<<BIO_RW_SYNCIO) | (1<<BIO_RW_UNPLUG);
>  
>  	bio->bi_bdev = rdev->bdev;
>  	bio->bi_sector = sector;
> @@ -531,7 +531,7 @@ int sync_page_io(struct block_device *bdev, sector_t sector, int size,
>  	struct completion event;
>  	int ret;
>  
> -	rw |= (1 << BIO_RW_SYNC);
> +	rw |= (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
>  
>  	bio->bi_bdev = bdev;
>  	bio->bi_sector = sector;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 2aa283a..1b16108 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -171,8 +171,6 @@ struct bio {
>  #define BIO_RW_FAILFAST_TRANSPORT	8
>  #define BIO_RW_FAILFAST_DRIVER		9
>  
> -#define BIO_RW_SYNC	(BIO_RW_SYNCIO | BIO_RW_UNPLUG)
> -
>  #define bio_rw_flagged(bio, flag)	((bio)->bi_rw & (1 << (flag)))
>  
>  /*
> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> index 25379cb..6e91587 100644
> --- a/include/linux/blktrace_api.h
> +++ b/include/linux/blktrace_api.h
> @@ -15,6 +15,7 @@ enum blktrace_cat {
>  	BLK_TC_WRITE	= 1 << 1,	/* writes */
>  	BLK_TC_BARRIER	= 1 << 2,	/* barrier */
>  	BLK_TC_SYNC	= 1 << 3,	/* sync IO */
> +	BLK_TC_SYNCIO	= BLK_TC_SYNC,
>  	BLK_TC_QUEUE	= 1 << 4,	/* queueing/merging */
>  	BLK_TC_REQUEUE	= 1 << 5,	/* requeueing */
>  	BLK_TC_ISSUE	= 1 << 6,	/* issue */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6022f44..67857dc 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -87,10 +87,10 @@ struct inodes_stat_t {
>  #define WRITE 1
>  #define READA 2		/* read-ahead  - don't block if no resources */
>  #define SWRITE 3	/* for ll_rw_block() - wait for buffer lock */
> -#define READ_SYNC	(READ | (1 << BIO_RW_SYNC))
> +#define READ_SYNC	(READ | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG))
>  #define READ_META	(READ | (1 << BIO_RW_META))
> -#define WRITE_SYNC	(WRITE | (1 << BIO_RW_SYNC))
> -#define SWRITE_SYNC	(SWRITE | (1 << BIO_RW_SYNC))
> +#define WRITE_SYNC	(WRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG))
> +#define SWRITE_SYNC	(SWRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG))
>  #define WRITE_BARRIER	(WRITE | (1 << BIO_RW_BARRIER))
>  #define DISCARD_NOBARRIER (1 << BIO_RW_DISCARD)
>  #define DISCARD_BARRIER ((1 << BIO_RW_DISCARD) | (1 << BIO_RW_BARRIER))
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 6da1435..505f319 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -60,6 +60,7 @@ static struct block_device *resume_bdev;
>  static int submit(int rw, pgoff_t page_off, struct page *page,
>  			struct bio **bio_chain)
>  {
> +	const int bio_rw = rw | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
>  	struct bio *bio;
>  
>  	bio = bio_alloc(__GFP_WAIT | __GFP_HIGH, 1);
> @@ -80,7 +81,7 @@ static int submit(int rw, pgoff_t page_off, struct page *page,
>  	bio_get(bio);
>  
>  	if (bio_chain == NULL) {
> -		submit_bio(rw | (1 << BIO_RW_SYNC), bio);
> +		submit_bio(bio_rw, bio);
>  		wait_on_page_locked(page);
>  		if (rw == READ)
>  			bio_set_pages_dirty(bio);
> @@ -90,7 +91,7 @@ static int submit(int rw, pgoff_t page_off, struct page *page,
>  			get_page(page);	/* These pages are freed later */
>  		bio->bi_private = *bio_chain;
>  		*bio_chain = bio;
> -		submit_bio(rw | (1 << BIO_RW_SYNC), bio);
> +		submit_bio(bio_rw, bio);
>  	}
>  	return 0;
>  }
> diff --git a/mm/page_io.c b/mm/page_io.c
> index dc6ce0a..3023c47 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -111,7 +111,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
>  		goto out;
>  	}
>  	if (wbc->sync_mode == WB_SYNC_ALL)
> -		rw |= (1 << BIO_RW_SYNC);
> +		rw |= (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
>  	count_vm_event(PSWPOUT);
>  	set_page_writeback(page);
>  	unlock_page(page);
> 
> -- 
> Jens Axboe

-- 
MST

Re: [Bug 12713] Hang on resume from hibernation, apparently as result of 213d9417fec

[Jens Axboe]

On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
> On Mon, Feb 16, 2009 at 09:47:32AM +0100, Jens Axboe wrote:
> > On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
> > > On Mon, Feb 16, 2009 at 9:27 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> > > > On Mon, Feb 16 2009, Michael S. Tsirkin wrote:
> > > >> On Mon, Feb 16, 2009 at 2:05 AM, Michael S. Tsirkin
> > > >> <m.s.tsirkin@gmail.com> wrote:
> > > >> > Summary: seem to need to revert 213d9417fec62ef4c3675621b9364a667954d4dd
> > > >> > to fix resume from hibernation. Bugzilla entry created:
> > > >> > http://bugzilla.kernel.org/show_bug.cgi?id=12713
> > > >>
> > > >> Looking over this, I see something strange in the commit in question:
> > > >>
> > > >> diff --git a/include/linux/bio.h b/include/linux/bio.h
> > > >> index 5175aa3..f53568c 100644
> > > >> --- a/include/linux/bio.h
> > > >> +++ b/include/linux/bio.h
> > > >> @@ -163,12 +163,15 @@ struct bio {
> > > >>  #define BIO_RW         0       /* Must match RW in req flags (blkdev.h) */
> > > >>  #define BIO_RW_AHEAD   1       /* Must match FAILFAST in req flags */
> > > >>  #define BIO_RW_BARRIER 2
> > > >> -#define BIO_RW_SYNC    3
> > > >> -#define BIO_RW_META    4
> > > >> -#define BIO_RW_DISCARD 5
> > > >> -#define BIO_RW_FAILFAST_DEV            6
> > > >> -#define BIO_RW_FAILFAST_TRANSPORT      7
> > > >> -#define BIO_RW_FAILFAST_DRIVER         8
> > > >> +#define BIO_RW_SYNCIO  3
> > > >> +#define BIO_RW_UNPLUG  4
> > > >> +#define BIO_RW_META    5
> > > >> +#define BIO_RW_DISCARD 6
> > > >> +#define BIO_RW_FAILFAST_DEV            7
> > > >> +#define BIO_RW_FAILFAST_TRANSPORT      8
> > > >> +#define BIO_RW_FAILFAST_DRIVER         9
> > > >> +
> > > >> +#define BIO_RW_SYNC    (BIO_RW_SYNCIO | BIO_RW_UNPLUG)
> > > >>
> > > >>  /*
> > > >>   * upper 16 bits of bi_rw define the io priority of this bio
> > > >>
> > > >> I haven't read the code in depth, but taking running numbers and doing
> > > >> bitwise "or"
> > > >> on them looks a bit strange to me.
> > > >> So here BIO_RW_SYNC is (3 | 4) = 7, that is the same as BIO_RW_FAILFAST_DEV.
> > > >> So for example bio_failfast_dev and bio_sync are the same.
> > > >>
> > > >> Jens, could you comment on this please? Is this intentional?
> > > >
> > > > That's clearly a braino, you can't OR the shift values of course. I'll
> > > > get it fixed up asap!
> > > 
> > > Cool, send me a patch, I'll test.
> > 
> > Try this. Not a huge fan of this approach, but it should bring behaviour
> > back to what it used to be.
> 
> I verified that applying this patch on top of 2.6.29-rc5 fixes
> hibernation for me.

Thanks for testing and confirming, it's queued up for inclusion asap.

-- 
Jens Axboe