ide write barrier support

drivers/ide/ide-io.c +22 -0

+/*
+ * preempt pending requests, and store this cache flush for immediate
+ * execution
+ */
+static struct request *ide_queue_flush_cmd(ide_drive_t *drive,
+					   struct request *rq, int post)
+{
+	struct request *flush_rq = &HWGROUP(drive)->wrq;
+
+	blkdev_dequeue_request(rq);
+
+	memset(drive->special_buf, 0, sizeof(drive->special_buf));
+
+	ide_init_drive_cmd(flush_rq);
+
+	flush_rq->buffer = drive->special_buf;
+	flush_rq->special = rq;
+	flush_rq->buffer[0] = WIN_FLUSH_CACHE;
+
+	if (drive->id->cfs_enable_2 & 0x2400)
+		flush_rq->buffer[0] = WIN_FLUSH_CACHE_EXT;
+

Quoted 22 lines · Jens Axboe

drivers/ide/ide-io.c +22 -0

+/*
+ * preempt pending requests, and store this cache flush for immediate
+ * execution
+ */
+static struct request *ide_queue_flush_cmd(ide_drive_t *drive,
+					   struct request *rq, int post)
+{
+	struct request *flush_rq = &HWGROUP(drive)->wrq;
+
+	blkdev_dequeue_request(rq);
+
+	memset(drive->special_buf, 0, sizeof(drive->special_buf));
+
+	ide_init_drive_cmd(flush_rq);
+
+	flush_rq->buffer = drive->special_buf;
+	flush_rq->special = rq;
+	flush_rq->buffer[0] = WIN_FLUSH_CACHE;
+
+	if (drive->id->cfs_enable_2 & 0x2400)
+		flush_rq->buffer[0] = WIN_FLUSH_CACHE_EXT;
+

drivers/ide/ide-io.c line 119

Quoted 10 lines

drivers/ide/ide-io.c +10 -0

+	if (!post) {
+		drive->doing_barrier = 1;
+		flush_rq->flags |= REQ_BAR_PREFLUSH;
+	} else
+		flush_rq->flags |= REQ_BAR_POSTFLUSH;
+
+	flush_rq->flags |= REQ_STARTED;
+	list_add(&flush_rq->queuelist, &drive->queue->queue_head);
+	return flush_rq;
+}

AFAICS you're missing some code that could be a major data corrupter:

FLUSH CACHE [EXT] may return before it's complete. You need to create an issue loop, that does FLUSH CACHE [EXT] and reads the result. If the result indicates the flush cache was partial, then you need to re-issue the flush. Lather, rinse, repeat until flush cache indicates all data is really flushed.

Quoted 36 lines · Jeff Garzik

Quoted 22 lines · Jens Axboe

drivers/ide/ide-io.c +22 -0

+/*
+ * preempt pending requests, and store this cache flush for immediate
+ * execution
+ */
+static struct request *ide_queue_flush_cmd(ide_drive_t *drive,
+					   struct request *rq, int post)
+{
+	struct request *flush_rq = &HWGROUP(drive)->wrq;
+
+	blkdev_dequeue_request(rq);
+
+	memset(drive->special_buf, 0, sizeof(drive->special_buf));
+
+	ide_init_drive_cmd(flush_rq);
+
+	flush_rq->buffer = drive->special_buf;
+	flush_rq->special = rq;
+	flush_rq->buffer[0] = WIN_FLUSH_CACHE;
+
+	if (drive->id->cfs_enable_2 & 0x2400)
+		flush_rq->buffer[0] = WIN_FLUSH_CACHE_EXT;
+

drivers/ide/ide-io.c line 119

Quoted 10 lines

drivers/ide/ide-io.c +10 -0

+	if (!post) {
+		drive->doing_barrier = 1;
+		flush_rq->flags |= REQ_BAR_PREFLUSH;
+	} else
+		flush_rq->flags |= REQ_BAR_POSTFLUSH;
+
+	flush_rq->flags |= REQ_STARTED;
+	list_add(&flush_rq->queuelist, &drive->queue->queue_head);
+	return flush_rq;
+}

AFAICS you're missing some code that could be a major data corrupter:

Quoted 5 lines

FLUSH CACHE [EXT] may return before it's complete. You need to create an issue loop, that does FLUSH CACHE [EXT] and reads the result. If the result indicates the flush cache was partial, then you need to re-issue the flush. Lather, rinse, repeat until flush cache indicates all data is really flushed.

It looks like you are right, at least the wording has changed since ata5 that states that BSY must remain set until all data has been flushed out (or error occurs). Which seems sane.

Only in the error case can I see this making sense, with partial flushes. It needs fixing (the error case too), but I'd hardly call that a major data corrupter. Not for the general case, we'd have to do really badly to risk corrupting data when compared to how 2.4 and 2.6 with journalling works now.

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

- Detect write cache setting and only issue SYNC_CACHE if write cache is enabled (not a biggy, all drives ship with it enabled)

Quoted 6 lines · Jens Axboe

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

- Detect write cache setting and only issue SYNC_CACHE if write cache is enabled (not a biggy, all drives ship with it enabled)

Yup, and I disable it on all drives at boot time at the latest.

Is there a status document that lists

- what SCSI drivers support write barriers
(I'm interested in sym53c8xx_2 if that matters)

- what IDE drivers support write barriers
(VIA for AMD and Intel for PII/PIII/P4 chip sets here)

Quoted 15 lines · Matthias Andree

Quoted 6 lines · Jens Axboe

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

- Detect write cache setting and only issue SYNC_CACHE if write cache is enabled (not a biggy, all drives ship with it enabled)

Yup, and I disable it on all drives at boot time at the latest.

Is there a status document that lists

Quoted 5 lines

- what SCSI drivers support write barriers
(I'm interested in sym53c8xx_2 if that matters)

- what IDE drivers support write barriers
(VIA for AMD and Intel for PII/PIII/P4 chip sets here)

The device is the entity that does, or does not, support flush-cache...
All IDE chipsets support flush-cache... it's just another IDE command.

Quoted 24 lines · Jeff Garzik

Quoted 21 lines · Matthias Andree

Quoted 6 lines · Jens Axboe

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

- Detect write cache setting and only issue SYNC_CACHE if write cache is enabled (not a biggy, all drives ship with it enabled)

Yup, and I disable it on all drives at boot time at the latest.

Is there a status document that lists

- what SCSI drivers support write barriers
(I'm interested in sym53c8xx_2 if that matters)

- what IDE drivers support write barriers (VIA for AMD and Intel for PII/PIII/P4 chip sets here)

Quoted 1 line

The device is the entity that does, or does not, support flush-cache... All IDE chipsets support flush-cache... it's just another IDE command.

Well drivers need to support it, too. IDE is supported, and that covers all devices that support it. It's not implemented on SCSI at all right now, that's coming up.

Quoted 26 lines · Jens Axboe

Quoted 24 lines · Jeff Garzik

Quoted 21 lines · Matthias Andree

Quoted 6 lines · Jens Axboe

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

- Detect write cache setting and only issue SYNC_CACHE if write cache is enabled (not a biggy, all drives ship with it enabled)

Yup, and I disable it on all drives at boot time at the latest.

Is there a status document that lists

- what SCSI drivers support write barriers
(I'm interested in sym53c8xx_2 if that matters)

- what IDE drivers support write barriers (VIA for AMD and Intel for PII/PIII/P4 chip sets here)

Quoted 7 lines

Quoted 2 lines

The device is the entity that does, or does not, support flush-cache... All IDE chipsets support flush-cache... it's just another IDE command.

Well drivers need to support it, too. IDE is supported, and that covers all devices that support it. It's not implemented on SCSI at all right now, that's coming up.

I do not see the need for write barrier support in drivers/ide/pci/{via82cxxx,piix,}.c, which is the question the original poster was asking.

Low-level chipset drivers should _not_ need to support it, the subsystem can do that.

Hi,

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

- Detect write cache setting and only issue SYNC_CACHE if write cache is enabled (not a biggy, all drives ship with it enabled)

- Toggle flush support on hdparm -W0/1

- Various small bits I can't remember right now

...

Quoted 16 lines · Jens Axboe

Hi,

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

- Detect write cache setting and only issue SYNC_CACHE if write cache is enabled (not a biggy, all drives ship with it enabled)

- Toggle flush support on hdparm -W0/1

- Various small bits I can't remember right now

...

Review

Quoted 1 line

(quoted diff) +1 -0

+		set_bit(BH_Ordered, &bh->b_state);

We have standard macros for generating standard buffer_head operations, so this can become

	set_buffer_ordered(bh);

See appended patch.

Quoted 6 lines

fs/jbd/commit.c +0 -0

@@ -474,7 +474,9 @@
 				clear_buffer_dirty(bh);
 				set_buffer_uptodate(bh);
 				bh->b_end_io = journal_end_buffer_io_sync;

fs/jbd/commit.c start_journal_io: · line 481

Quoted 5 lines

fs/jbd/commit.c +2 -0

+				set_bit(BH_Ordered, &bh->b_state);
 				submit_bh(WRITE, bh);
+				clear_bit(BH_Ordered, &bh->b_state);
 			}
 			cond_resched();

Why does the ordering go here? I'd have thought that we only need to enforce ordering around the commit block.

Touching the bh here after submitting it may be racy: may need to take an extra ref against the bh to prevent it from disappearing. I need to look at it more closely.

fs/jbd/commit.c line 355

Quoted 9 lines

fs/jbd/commit.c +2 -0

@@ -344,6 +348,8 @@
 	unsigned long		seg_boundary_mask;
 	unsigned int		dma_alignment;
 
+	unsigned short		ordered;
+
 	struct blk_queue_tag	*queue_tags;
 
 	atomic_t		refcnt;

shorts-in-structs worry me. If the CPU implements a write-to-short as a word-sized RMW and the compiler decides to align or pack the short into a less-than-wored-sized storage space then a write-to-short could stomp on a neighbouring member.

I doubt if it can happen, but if so, I'd be interested in knowing what guarantees it.

fs/jbd/commit.c

Quoted 3 lines

...

fs/jbd/commit.c +1 -0

 	unsigned vdma		: 1;	/* 1=doing PIO over DMA 0=doing normal DMA */
+	unsigned doing_barrier	: 1;	/* state, 1=currently doing flush */

Similarly, I suspect that bitfields like this need locking. If the CPU implements a write-to-bitfield as a non-buslocked RMW it can stomp on neighbouring bitfields in the same word.

Hi,

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

Quoted 5 lines · Jens Axboe

Hi,

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest. TODO:

Is there a user-space interface planned for this?

One possibility may be just to hang it off fsync(2) so fsync doesn't return until until all the buffers it flushed are actually synced to disk. That's its documented semantics anyways.

There's also the case of files opened with O_SYNC. Would inserting a write barrier after every write to such a file destroy performance?

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest.

From a practical point of view the only immediate use we have for barriers is

Hi Jens,

Quoted 3 lines · Jens Axboe

Forward ported and tested today (with the dummy ext3 patch included), works for me. Some todo's left, but I thought I'd send it out to gauge interest.

This is highly interesting of course, but is it suitable for submission during the stability freeze? There is no correctness issue so long as no filesystem in mainline sets the BIO_RW_BARRIER bit, which appears to be the case. Therefore this is really a performance patch that introduces a new internal API.

It seems to me there are a few unresolved issues with the barrier API. It

needs to be clearly stated that only write barriers are supported, not read or read/write barriers, if that is in fact the intention. Assuming it is, then BIOs with read barriers need to be failed.

The current BIO API provides no way to express a rw barrier, only read barriers and write barriers (the combination of direction bit and barrier bit indicates the barrier type). This is minor but it but how nice it would be if the API was either orthogonal or there was a clear explanation of why RW barriers never make sense. And if they don't, why read barriers do make sense. Another possible wart is that the API doesn't allow for a read barrier carried by a write BIO or a write barrier carried by a read BIO. From a practical point of view the only immediate use we have for barriers is to accelerate journal writes and everything else comes under the heading of R&D. It would help if the code clearly reflected that modest goal.

The BIO barrier scheme doesn't mesh properly with your proposed QUEUE_ORDERED_* scheme. It seems to me that what you want is just QUEUE_ORDERED_NONE and QUEUE_ORDERED_WRITE. Is there any case where the distinction between a tag based implemenation versus a flush matters to high level code?

Also, the blk_queue_ordered function isn't a sufficient interface to enable the functionality at a high level, a filesystem also needs a way to know whether barriers are supported or not, short of just submitting a barrier request and seeing if it fails.

The high level interface needs to be able to handled stacked devices, i.e., device mapper, but not just device mapper. Barriers have to be supported by all the devices in the stack, not just the top or bottom one. I don't have a concrete suggestion on what the interface should be just now.

The point of this is, there still remain a number of open issues with this patch, no doubt more than just the ones I touched on. Though it is clearly headed in the right direction, I'd suggest holding off during the stability freeze and taking the needed time to get it right.

There IS a correctness issue, in
that drives ship with write back caching enabled. The fs assumes that once wait_on_buffer() returns, the data is on disk. Which is false, can remain false for quite a long number of seconds.

Quoted 4 lines · Jens Axboe

There IS a correctness issue, in
that drives ship with write back caching enabled. The fs assumes that once wait_on_buffer() returns, the data is on disk. Which is false, can remain false for quite a long number of seconds.

Maybe what you're saying is, there are only two ways to deal with IDE drives with non-disablable writeback cache:

1) flush the cache on every write
2) Implement write barriers, add them to all the journalling filesystems, and flush only on the write barrier

and (1) is just too slow. Correct?

Quoted 7 lines · Daniel Phillips

Maybe what you're saying is, there are only two ways to deal with IDE drives with non-disablable writeback cache:

1) flush the cache on every write
2) Implement write barriers, add them to all the journalling
filesystems, and flush only on the write barrier

Quoted 1 line

and (1) is just too slow. Correct?

Yes, that is exactly what I'm saying. It's not just that, though. Completely disabling write back caching on IDE drives totally kills performance typically, they are just not meant to be driven this way.

Quoted 10 lines · Jens Axboe

Quoted 8 lines · Daniel Phillips

Maybe what you're saying is, there are only two ways to deal with IDE drives with non-disablable writeback cache:

1) flush the cache on every write
2) Implement write barriers, add them to all the journalling
filesystems, and flush only on the write barrier

and (1) is just too slow. Correct?

Quoted 3 lines

Yes, that is exactly what I'm saying. It's not just that, though. Completely disabling write back caching on IDE drives totally kills performance typically, they are just not meant to be driven this way.

OK, this is still experimental development in the middle of the stability freeze in order to fix a performance bug, but that's a tradition anyway so let me join in.

I'm specifically interested in working out the issues related to stacked virtual devices, and there are many. Let me start with an easy one.

Consider a multipath virtual device that is doing load balancing and wants to handle write barriers efficiently, not just allow the downstream queues to drain before allowing new writes. This device wants to send a write barrier to each of the downstream devices, however, we have only one write request to carry the barrier bit. How do you recommend handling this situation?

Quoted 9 lines · Daniel Phillips

I'm specifically interested in working out the issues related to stacked virtual devices, and there are many. Let me start with an easy one.

Consider a multipath virtual device that is doing load balancing and wants to handle write barriers efficiently, not just allow the downstream queues to drain before allowing new writes. This device wants to send a write barrier to each of the downstream devices, however, we have only one write request to carry the barrier bit. How do you recommend handling this situation?

That needs something to hold the state in, and a bio per device. As they complete, mark them as such. When they all have completed, barrier is done.

That's just an idea, I'm sure there are other ways. Depending on how complex it gets, it might not be a bad idea to just let the queues drain though. I think I'd prefer that approach.

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Daniel Phillips

I'm specifically interested in working out the issues related to stacked virtual devices, and there are many. Let me start with an easy one.

Consider a multipath virtual device that is doing load balancing and wants to handle write barriers efficiently, not just allow the downstream queues to drain before allowing new writes. This device wants to send a write barrier to each of the downstream devices, however, we have only one write request to carry the barrier bit. How do you recommend handling this situation?

Quoted 8 lines

That needs something to hold the state in, and a bio per device. As they complete, mark them as such. When they all have completed, barrier is done.

That's just an idea, I'm sure there are other ways. Depending on how complex it gets, it might not be a bad idea to just let the queues drain though. I think I'd prefer that approach.

These are essentially the same, they both rely on draining the downstream queues. But if we could keep the downstream queues full, bus transfers for post-barrier writes will overlap the media transfers for pre-barrier writes, which would seem to be worth some extra effort.

To keep the downstream queues full, we must submit write barriers to all the downstream devices and not wait for completion. That is, as soon as a barrier is issued to a given downstream device we can start passing through post-barrier writes to it.

Assuming this is worth doing, how do we issue N barriers to the downstream devices when we have only one incoming barrier write?

Quoted 7 lines · Daniel Phillips

To keep the downstream queues full, we must submit write barriers to all the downstream devices and not wait for completion. That is, as soon as a barrier is issued to a given downstream device we can start passing through post-barrier writes to it.

Assuming this is worth doing, how do we issue N barriers to the downstream devices when we have only one incoming barrier write?

You would do this in the multipath code, wouldn't you?

Anyway, I might be missing something, but I don't think draining the queue will guarantee that writeback caches will go to permanent storage.

Not entirely within the multipath virtual device, that's the problem. If it could stay somehow all within one device driver then ok, but since we want to build this modularly, as a device mapper target, there are API issues.

On 2003-10-26T23:06:53,

Quoted 4 lines · Daniel Phillips

Not entirely within the multipath virtual device, that's the problem. If it could stay somehow all within one device driver then ok, but since we want to build this modularly, as a device mapper target, there are API issues.

Are you seeing problems with the write-ordering properties of multipathing? If so, what is the issue with handling them in the DM target once?

These are essentially the same, they both rely on draining the downstream queues. But if we could keep the downstream queues full, bus transfers for post-barrier writes will overlap the media transfers for pre-barrier writes, which would seem to be worth some extra effort.

To keep the downstream queues full, we must submit write barriers to all the downstream devices and not wait for completion. That is, as soon as a barrier is issued to a given downstream device we can start passing through post-barrier writes to it.

Quoted 9 lines · Daniel Phillips

These are essentially the same, they both rely on draining the downstream queues. But if we could keep the downstream queues full, bus transfers for post-barrier writes will overlap the media transfers for pre-barrier writes, which would seem to be worth some extra effort.

To keep the downstream queues full, we must submit write barriers to all the downstream devices and not wait for completion. That is, as soon as a barrier is issued to a given downstream device we can start passing through post-barrier writes to it.

This approach may fail:

a. Some pre-barrier writes go to all devices
b. barrier is sent to all devices
c. Post-barrier writes go to all devices
d. drive 1 commits all its pre-barrier writes, then
commits its post-barrier writes.
e. drive 2 is slow and havent done the pre-barrier writes yet f. power is lost - leaving inconsistent devices.

The problem is that drive 1 don't know wether drive 2 did the barrier yet.