Bug 12713
| Summary: | Hang on resume from hibernation, apparently as result of 213d9417fec | ||
|---|---|---|---|
| Product: | IO/Storage | Reporter: | Michael S. Tsirkin (m.s.tsirkin) |
| Component: | Block Layer | Assignee: | Jens Axboe (axboe) |
| Status: | CLOSED CODE_FIX | ||
| Severity: | normal | CC: | maekke, pavel, rjw |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 2.6.29-rc5 | Subsystem: | |
| Regression: | Yes | Bisected commit-id: | |
| Bug Depends on: | |||
| Bug Blocks: | 7216, 12398 | ||
| Attachments: |
.config for 2.6.29-rc5
Patch from Jens: I verified that it fixes hibernation |
||
|
Description
Michael S. Tsirkin
2009-02-15 11:34:26 UTC
Created attachment 20256 [details]
.config for 2.6.29-rc5
argh, why did you duplicate 12710? Oh well, Rafael will sort if out :) First-Bad-Commit : 213d9417fec62ef4c3675621b9364a667954d4dd *** Bug 12710 has been marked as a duplicate of this bug. *** 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
Maybe this is intentional, 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?
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 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? Reply-To: rdreier@cisco.com > 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. Reply-To: jens.axboe@oracle.com 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! 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 > > Reply-To: jens.axboe@oracle.com 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); 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 Created attachment 20263 [details]
Patch from Jens: I verified that it fixes hibernation
Handled-By : Jens Axboe <jens.axboe@oracle.com> Patch : http://bugzilla.kernel.org/show_bug.cgi?id=12713#c11 Reply-To: jens.axboe@oracle.com 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. Closing, patch is merged as commit 93dbb393503d53cd226e5e1f0088fe8f4dbaa2b8 |