Skip to contentSkip to conversation

block: fix DISCARD_BARRIER requests

PATCH Applied Christoph Hellwig 6 participants 10 messages Jun 17, 2010 – Jun 19, 2010

Subsystems (2)
VFS Block layer
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 block: fix DISCARD_BARRIER requests

Filesystems assume that DISCARD_BARRIER are full barriers, so that they don't have to track in-progress discard operation when submitting new I/O. But currently we only treat them as elevator barriers, which don't actually do the nessecary queue drains.

Also remove the unlikely around both the DISCARD and BARRIER requests - the happen far too often for a static mispredict.

Signed-off-by Christoph Hellwig <[email protected]>
block/blk-core.c +2 -5
Index: linux-2.6/block/blk-core.c
===================================================================
@@ -1149,13 +1149,10 @@ void init_request_from_bio(struct reques
 	else
 		req->cmd_flags |= bio->bi_rw & REQ_FAILFAST_MASK;
 
-	if (unlikely(bio_rw_flagged(bio, BIO_RW_DISCARD))) {
+	if (bio_rw_flagged(bio, BIO_RW_DISCARD))
 		req->cmd_flags |= REQ_DISCARD;
-		if (bio_rw_flagged(bio, BIO_RW_BARRIER))
-			req->cmd_flags |= REQ_SOFTBARRIER;
-	} else if (unlikely(bio_rw_flagged(bio, BIO_RW_BARRIER)))
+	if (bio_rw_flagged(bio, BIO_RW_BARRIER))
 		req->cmd_flags |= REQ_HARDBARRIER;
-
 	if (bio_rw_flagged(bio, BIO_RW_SYNCIO))
 		req->cmd_flags |= REQ_RW_SYNC;
 	if (bio_rw_flagged(bio, BIO_RW_META))
maintainer Applied
#1 lore raw
Quoted 7 lines · Christoph Hellwig

Filesystems assume that DISCARD_BARRIER are full barriers, so that they don't have to track in-progress discard operation when submitting new I/O. But currently we only treat them as elevator barriers, which don't actually do the nessecary queue drains.

Also remove the unlikely around both the DISCARD and BARRIER requests - the happen far too often for a static mispredict.

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

Signature · 1 line
Jens Axboe
#2 lore raw
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Christoph Hellwig

Filesystems assume that DISCARD_BARRIER are full barriers, so that they don't have to track in-progress discard operation when submitting new I/O. But currently we only treat them as elevator barriers, which don't actually do the nessecary queue drains.

Quoted 6 lines
Quoted 2 lines

Also remove the unlikely around both the DISCARD and BARRIER requests - the happen far too often for a static mispredict.

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

If it was on real hardware (ie a SATA disc), it can't be this problem, since TRIM isn't NCQ so there was already an implicit queue drain ahead and behind the TRIM. I hear rumours of an NCQ TRIM coming 'RSN', but haven't seen any details on it yet.

Signature · 4 lines
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
maintainer
#3 lore raw
Quoted 10 lines · Matthew Wilcox
Quoted 9 lines · Jens Axboe
Quoted 7 lines · Christoph Hellwig

Filesystems assume that DISCARD_BARRIER are full barriers, so that they don't have to track in-progress discard operation when submitting new I/O. But currently we only treat them as elevator barriers, which don't actually do the nessecary queue drains.

Also remove the unlikely around both the DISCARD and BARRIER requests - the happen far too often for a static mispredict.

Quoted 8 lines
Quoted 3 lines

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

If it was on real hardware (ie a SATA disc), it can't be this problem, since TRIM isn't NCQ so there was already an implicit queue drain ahead and behind the TRIM. I hear rumours of an NCQ TRIM coming 'RSN', but haven't seen any details on it yet.

Good point, it will drain the queue on SATA of course. The problem was reported on MMC, I doubt the do queuing at all (though I have not checked).

Signature · 1 line
Jens Axboe
author
#4 lore raw
In reply to Jens Axboe
Quoted 3 lines · Jens Axboe

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

We actually have a much bigger issue with the DISCARD_BARRIER type. If the discard request needs to get split into multiple smaller ones we don't keep the queue drained atomically around them, so requests could sneak inbetween them. Depending on how the realtime discard is implemented that could cause issues. In my XFS prototype for it I only deleted the extents from the tracking betree after the discard request has returned, but other filesystems rely on full barrier semantics of DISCARD_BARRIER this could cause real problems.

#5 lore raw
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Jens Axboe

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

Quoted 8 lines

We actually have a much bigger issue with the DISCARD_BARRIER type. If the discard request needs to get split into multiple smaller ones we don't keep the queue drained atomically around them, so requests could sneak inbetween them. Depending on how the realtime discard is implemented that could cause issues. In my XFS prototype for it I only deleted the extents from the tracking betree after the discard request has returned, but other filesystems rely on full barrier semantics of DISCARD_BARRIER this could cause real problems.

btrfs needs to know that a write after the discard returns won't cross the discard, but beyond that we're happy with anything.

-chris

#6 lore raw
Quoted 15 lines · Chris Mason
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Jens Axboe

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

Quoted 8 lines

We actually have a much bigger issue with the DISCARD_BARRIER type. If the discard request needs to get split into multiple smaller ones we don't keep the queue drained atomically around them, so requests could sneak inbetween them.  Depending on how the realtime discard is implemented that could cause issues.  In my XFS prototype for it I only deleted the extents from the tracking betree after the discard request has returned, but other filesystems rely on full barrier semantics of DISCARD_BARRIER this could cause real problems.

Quoted 2 lines

btrfs needs to know that a write after the discard returns won't cross the discard, but beyond that we're happy with anything.

I tested btrfs on MMC with discard support and get failed. you can find a scenario and details at btrfs mailing list.
I'll check it with this patch.
Please note that MMC don't have any queue at internal as SSD.

Thank you,
Kyungmin Park
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw
Quoted 16 lines · Kyungmin Park
Quoted 15 lines · Chris Mason
Quoted 13 lines · Christoph Hellwig
Quoted 3 lines · Jens Axboe

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

We actually have a much bigger issue with the DISCARD_BARRIER type. If the discard request needs to get split into multiple smaller ones we don't keep the queue drained atomically around them, so requests could sneak inbetween them.  Depending on how the realtime discard is implemented that could cause issues.  In my XFS prototype for it I only deleted the extents from the tracking betree after the discard request has returned, but other filesystems rely on full barrier semantics of DISCARD_BARRIER this could cause real problems.

Quoted 8 lines
Quoted 2 lines

btrfs needs to know that a write after the discard returns won't cross the discard, but beyond that we're happy with anything.

I tested btrfs on MMC with discard support and get failed. you can find a scenario and details at btrfs mailing list.
I'll check it with this patch.
Please note that MMC don't have any queue at internal as SSD.

It's same it failed to use discard on MMC with btrfs.

Thank you,
Kyungmin Park

Quoted 1 line

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#8 lore raw
In reply to Chris Mason
Quoted 15 lines · Chris Mason
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Jens Axboe

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

Quoted 8 lines

We actually have a much bigger issue with the DISCARD_BARRIER type. If the discard request needs to get split into multiple smaller ones we don't keep the queue drained atomically around them, so requests could sneak inbetween them. Depending on how the realtime discard is implemented that could cause issues. In my XFS prototype for it I only deleted the extents from the tracking betree after the discard request has returned, but other filesystems rely on full barrier semantics of DISCARD_BARRIER this could cause real problems.

Quoted 2 lines

btrfs needs to know that a write after the discard returns won't cross the discard, but beyond that we're happy with anything.

Is it acceptable for the write to move earlier than a discard that it doesn't overlap? In other words, would a range-dependent barrier be sufficient (hypothetically, for some future elevator / multi-disk optimisation).

I guess answer to that depends on whether you're queuing a metadata write to record some fact about the discard which shouldn't reach the storage until the discard is confirmed done.

-- Jamie

#9 lore raw
Quoted 19 lines · Jamie Lokier
Quoted 15 lines · Chris Mason
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Jens Axboe

Thanks, applied. There was a recent problem report on btrfs using discard, could possibly explain it if Chris assumed it was a full barrier.

Quoted 8 lines

We actually have a much bigger issue with the DISCARD_BARRIER type. If the discard request needs to get split into multiple smaller ones we don't keep the queue drained atomically around them, so requests could sneak inbetween them. Depending on how the realtime discard is implemented that could cause issues. In my XFS prototype for it I only deleted the extents from the tracking betree after the discard request has returned, but other filesystems rely on full barrier semantics of DISCARD_BARRIER this could cause real problems.

Quoted 2 lines

btrfs needs to know that a write after the discard returns won't cross the discard, but beyond that we're happy with anything.

Quoted 8 lines

Is it acceptable for the write to move earlier than a discard that it doesn't overlap? In other words, would a range-dependent barrier be sufficient (hypothetically, for some future elevator / multi-disk optimisation).

I guess answer to that depends on whether you're queuing a metadata write to record some fact about the discard which shouldn't reach the storage until the discard is confirmed done.

It's really just the sector we're discarding that matters. So if I discard sector xxyyzz and then write the same sector, please make sure the discard is done before you put down my new contents ;)

-chris