request to revert libata-convert-to-block-tagging patches

request to revert libata-convert-to-block-tagging patches

[Tejun Heo]

Hello, all.

I went through libata-convert-to-block-tagging today and found several
issues.

1. libata internal data structure for command context (qc) allocation is
bound to tag allocation, which means that block layer tagging should be
enabled for all controllers which have can_queue > 1.

2. blk-tag offsets allocation for non-sync requests.  I'm not confident
this is safe.  Till now, if there was only single command in flight for
the port, it was guaranteed that the qc gets tag zero whether the device
is NCQ capable or not.  qc allocation is tied tightly with hardware
command slot allocation and I don't think it's wise to change this
assumption.

#1 is easy to fix but #2 requires either adding a spinlock or two atomic
variables to struct blk_queue_tag to keep the current behavior while
guaranteeing that tags are used in order.  Also, there's delay between
libata marks a request complete and the request actually gets completed
and the tag is freed.  If another request gets issued inbetween, the tag
number can't be guaranteed.  This can be worked around by re-mapping tag
number in libata depending on command type but, well then, it's worse
than the original implementation.

So, please revert the following commits.

43a49cbdf31e812c0d8f553d433b09b421f5d52c
e013e13bf605b9e6b702adffbe2853cfc60e7806
2fca5ccf97d2c28bcfce44f5b07d85e74e3cd18e

Thanks.

-- 
tejun

Re: request to revert libata-convert-to-block-tagging patches

[Tejun Heo]

Tejun Heo wrote:
> #1 is easy to fix but #2 requires either adding a spinlock or two atomic
> variables to struct blk_queue_tag to keep the current behavior while
> guaranteeing that tags are used in order.  Also, there's delay between
> libata marks a request complete and the request actually gets completed
> and the tag is freed.  If another request gets issued inbetween, the tag
> number can't be guaranteed.  This can be worked around by re-mapping tag
> number in libata depending on command type but, well then, it's worse
> than the original implementation.

I think this can be made to work by making tag free synchronous (ie.
doing it when the request is passed over to softirq) but I don't think
things like that should go into 2.6.28 at this point.

Thanks.

-- 
tejun

[PATCH] libata: revert convert-to-block-tagging patches

[Tejun Heo]

This patch reverts the following three commits which convert libata to
use block layer tagging.

 43a49cbdf31e812c0d8f553d433b09b421f5d52c
 e013e13bf605b9e6b702adffbe2853cfc60e7806
 2fca5ccf97d2c28bcfce44f5b07d85e74e3cd18e

Although using block layer tagging is the right direction, due to the
tight coupling among tag number, data structure allocation and
hardware command slot allocation, libata doesn't work correctly with
the current conversion.

The biggest problem is guaranteeing that tag 0 is always used for
non-NCQ commands.  Due to the way blk-tag is implemented and how SCSI
starts and finishes requests, such guarantee can't be made.  I'm not
sure whether this would actually break any low level driver but it
doesn't look like a good idea to break such assumption given the
frailty of ATA controllers.

So, for the time being, keep using the old dumb in-libata qc
allocation.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axobe <jens.axboe@oracle.com>
Cc: Jeff Garzik <jeff@garzik.org>
---
 drivers/ata/libata-core.c |   66 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/ata/libata-scsi.c |   23 +---------------
 drivers/ata/libata.h      |   19 +------------
 include/linux/libata.h    |    1 
 4 files changed, 65 insertions(+), 44 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 622350d..0cd3ad4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1712,6 +1712,8 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	else
 		tag = 0;
 
+	if (test_and_set_bit(tag, &ap->qc_allocated))
+		BUG();
 	qc = __ata_qc_from_tag(ap, tag);
 
 	qc->tag = tag;
@@ -4563,6 +4565,37 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
 }
 
 /**
+ *	ata_qc_new - Request an available ATA command, for queueing
+ *	@ap: Port associated with device @dev
+ *	@dev: Device from whom we request an available command structure
+ *
+ *	LOCKING:
+ *	None.
+ */
+
+static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = NULL;
+	unsigned int i;
+
+	/* no command while frozen */
+	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
+		return NULL;
+
+	/* the last tag is reserved for internal command. */
+	for (i = 0; i < ATA_MAX_QUEUE - 1; i++)
+		if (!test_and_set_bit(i, &ap->qc_allocated)) {
+			qc = __ata_qc_from_tag(ap, i);
+			break;
+		}
+
+	if (qc)
+		qc->tag = i;
+
+	return qc;
+}
+
+/**
  *	ata_qc_new_init - Request an available ATA command, and initialize it
  *	@dev: Device from whom we request an available command structure
  *	@tag: command tag
@@ -4571,20 +4604,16 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
  *	None.
  */
 
-struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
+struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev)
 {
 	struct ata_port *ap = dev->link->ap;
 	struct ata_queued_cmd *qc;
 
-	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
-		return NULL;
-
-	qc = __ata_qc_from_tag(ap, tag);
+	qc = ata_qc_new(ap);
 	if (qc) {
 		qc->scsicmd = NULL;
 		qc->ap = ap;
 		qc->dev = dev;
-		qc->tag = tag;
 
 		ata_qc_reinit(qc);
 	}
@@ -4592,6 +4621,31 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
 	return qc;
 }
 
+/**
+ *	ata_qc_free - free unused ata_queued_cmd
+ *	@qc: Command to complete
+ *
+ *	Designed to free unused ata_queued_cmd object
+ *	in case something prevents using it.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ */
+void ata_qc_free(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	unsigned int tag;
+
+	WARN_ON(qc == NULL);	/* ata_qc_from_tag _might_ return NULL */
+
+	qc->flags = 0;
+	tag = qc->tag;
+	if (likely(ata_tag_valid(tag))) {
+		qc->tag = ATA_TAG_POISON;
+		clear_bit(tag, &ap->qc_allocated);
+	}
+}
+
 void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3fa75ea..47c7afc 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -709,11 +709,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
 {
 	struct ata_queued_cmd *qc;
 
-	if (cmd->request->tag != -1)
-		qc = ata_qc_new_init(dev, cmd->request->tag);
-	else
-		qc = ata_qc_new_init(dev, 0);
-
+	qc = ata_qc_new_init(dev);
 	if (qc) {
 		qc->scsicmd = cmd;
 		qc->scsidone = done;
@@ -1108,17 +1104,7 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 
 		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
 		depth = min(ATA_MAX_QUEUE - 1, depth);
-
-		/*
-		 * If this device is behind a port multiplier, we have
-		 * to share the tag map between all devices on that PMP.
-		 * Set up the shared tag map here and we get automatic.
-		 */
-		if (dev->link->ap->pmp_link)
-			scsi_init_shared_tag_map(sdev->host, ATA_MAX_QUEUE - 1);
-
-		scsi_set_tag_type(sdev, MSG_SIMPLE_TAG);
-		scsi_activate_tcq(sdev, depth);
+		scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG, depth);
 	}
 
 	return 0;
@@ -1958,11 +1944,6 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
 		hdr[1] |= (1 << 7);
 
 	memcpy(rbuf, hdr, sizeof(hdr));
-
-	/* if ncq, set tags supported */
-	if (ata_id_has_ncq(args->id))
-		rbuf[7] |= (1 << 1);
-
 	memcpy(&rbuf[8], "ATA     ", 8);
 	ata_id_string(args->id, &rbuf[16], ATA_ID_PROD, 16);
 	ata_id_string(args->id, &rbuf[32], ATA_ID_FW_REV, 4);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index d3831d3..fe2839e 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -74,7 +74,7 @@ extern struct ata_link *ata_dev_phys_link(struct ata_device *dev);
 extern void ata_force_cbl(struct ata_port *ap);
 extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
 extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf);
-extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag);
+extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);
 extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 			   u64 block, u32 n_block, unsigned int tf_flags,
 			   unsigned int tag);
@@ -103,6 +103,7 @@ extern int ata_dev_configure(struct ata_device *dev);
 extern int sata_down_spd_limit(struct ata_link *link);
 extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
 extern void ata_sg_clean(struct ata_queued_cmd *qc);
+extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern void ata_qc_issue(struct ata_queued_cmd *qc);
 extern void __ata_qc_complete(struct ata_queued_cmd *qc);
 extern int atapi_check_dma(struct ata_queued_cmd *qc);
@@ -118,22 +119,6 @@ extern struct ata_port *ata_port_alloc(struct ata_host *host);
 extern void ata_dev_enable_pm(struct ata_device *dev, enum link_pm policy);
 extern void ata_lpm_schedule(struct ata_port *ap, enum link_pm);
 
-/**
- *	ata_qc_free - free unused ata_queued_cmd
- *	@qc: Command to complete
- *
- *	Designed to free unused ata_queued_cmd object
- *	in case something prevents using it.
- *
- *	LOCKING:
- *	spin_lock_irqsave(host lock)
- */
-static inline void ata_qc_free(struct ata_queued_cmd *qc)
-{
-	qc->flags = 0;
-	qc->tag = ATA_TAG_POISON;
-}
-
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
 extern void ata_acpi_associate_sata_port(struct ata_port *ap);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c7665a4..59b0f1c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -698,6 +698,7 @@ struct ata_port {
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
 
 	struct ata_queued_cmd	qcmd[ATA_MAX_QUEUE];
+	unsigned long		qc_allocated;
 	unsigned int		qc_active;
 	int			nr_active_links; /* #links with active qcs */
 

Re: request to revert libata-convert-to-block-tagging patches

[Jens Axboe]

On Mon, Nov 10 2008, Tejun Heo wrote:
> Hello, all.
> 
> I went through libata-convert-to-block-tagging today and found several
> issues.
> 
> 1. libata internal data structure for command context (qc) allocation is
> bound to tag allocation, which means that block layer tagging should be
> enabled for all controllers which have can_queue > 1.

Naturally, is there a problem there?

> 2. blk-tag offsets allocation for non-sync requests.  I'm not confident
> this is safe.  Till now, if there was only single command in flight for
> the port, it was guaranteed that the qc gets tag zero whether the device
> is NCQ capable or not.  qc allocation is tied tightly with hardware
> command slot allocation and I don't think it's wise to change this
> assumption.
> 
> #1 is easy to fix but #2 requires either adding a spinlock or two atomic
> variables to struct blk_queue_tag to keep the current behavior while
> guaranteeing that tags are used in order.  Also, there's delay between
> libata marks a request complete and the request actually gets completed
> and the tag is freed.  If another request gets issued inbetween, the tag
> number can't be guaranteed.  This can be worked around by re-mapping tag
> number in libata depending on command type but, well then, it's worse
> than the original implementation.

Or we could just change the blk-tag.c logic to stop of
find_first_zero_bit() returns >= some_value instead of starting at an
offset? You don't need any extra locking for that.

The second part is more tricky I think, but I'm not sure there's a race
there. For normally issued IO, queueing is restarted when the tag
completes. There's a small softirq delay there, but that delay is before
the tag is completed and queueing restarted. Any non-ncq command (eg
through an ioctl) will have to wait for completion as well.

> So, please revert the following commits.
> 
> 43a49cbdf31e812c0d8f553d433b09b421f5d52c
> e013e13bf605b9e6b702adffbe2853cfc60e7806
> 2fca5ccf97d2c28bcfce44f5b07d85e74e3cd18e

It's not a big deal to me, it can wait for 2.6.29 if there really are
more issues there. But I'm not sure that your points are very valid, so
lets please discuss it a bit more :-)

-- 
Jens Axboe

Re: request to revert libata-convert-to-block-tagging patches

[Jens Axboe]

On Mon, Nov 10 2008, Jens Axboe wrote:
> > 2. blk-tag offsets allocation for non-sync requests.  I'm not confident
> > this is safe.  Till now, if there was only single command in flight for
> > the port, it was guaranteed that the qc gets tag zero whether the device
> > is NCQ capable or not.  qc allocation is tied tightly with hardware
> > command slot allocation and I don't think it's wise to change this
> > assumption.
> > 
> > #1 is easy to fix but #2 requires either adding a spinlock or two atomic
> > variables to struct blk_queue_tag to keep the current behavior while
> > guaranteeing that tags are used in order.  Also, there's delay between
> > libata marks a request complete and the request actually gets completed
> > and the tag is freed.  If another request gets issued inbetween, the tag
> > number can't be guaranteed.  This can be worked around by re-mapping tag
> > number in libata depending on command type but, well then, it's worse
> > than the original implementation.
> 
> Or we could just change the blk-tag.c logic to stop of
> find_first_zero_bit() returns >= some_value instead of starting at an
> offset? You don't need any extra locking for that.

Something like the below.

diff --git a/block/blk-tag.c b/block/blk-tag.c
index c0d419e..451e7ce 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -337,7 +337,7 @@ EXPORT_SYMBOL(blk_queue_end_tag);
 int blk_queue_start_tag(struct request_queue *q, struct request *rq)
 {
 	struct blk_queue_tag *bqt = q->queue_tags;
-	unsigned max_depth, offset;
+	unsigned max_depth;
 	int tag;
 
 	if (unlikely((rq->cmd_flags & REQ_QUEUED))) {
@@ -356,13 +356,11 @@ int blk_queue_start_tag(struct request_queue *q, struct request *rq)
 	 * to starve sync IO on behalf of flooding async IO.
 	 */
 	max_depth = bqt->max_depth;
-	if (rq_is_sync(rq))
-		offset = 0;
-	else
-		offset = max_depth >> 2;
+	if (!rq_is_sync(rq))
+		max_depth = 3 * max_depth / 4;
 
 	do {
-		tag = find_next_zero_bit(bqt->tag_map, max_depth, offset);
+		tag = find_first_zero_bit(bqt->tag_map, max_depth);
 		if (tag >= max_depth)
 			return 1;
 

-- 
Jens Axboe

Re: request to revert libata-convert-to-block-tagging patches

[Linus Torvalds]

On Mon, 10 Nov 2008, Jens Axboe wrote:
> > 
> > Or we could just change the blk-tag.c logic to stop of
> > find_first_zero_bit() returns >= some_value instead of starting at an
> > offset? You don't need any extra locking for that.
> 
> Something like the below.

No, there were two reasons for doing it the way I did it, and this shows 
both. One trivial, one subtle.

> +	if (!rq_is_sync(rq))
> +		max_depth = 3 * max_depth / 4;

The trivial one here is that you round down. Imagine what happens if 
"max_depth" was 1.

The subtler one was that the 'use starting offset' means that async and 
sync can _share_ the tagspace, and while you limit async ones to a maximum 
outstanding number, you really cut down on them only when sync ones really 
have filled everything up.

In contrast, limiting like the above means that it's much easier to be in 
the situation where you still have tags to use, but you've used them all 
for reads, and you refuse to start a single write.

Anyway, I'll do the revert, since -rc4 is too late to discuss these 
issues. I think we can easily re-do things when everybody is ok with the 
code.

			Linus

Re: request to revert libata-convert-to-block-tagging patches

[Jens Axboe]

On Mon, Nov 10 2008, Linus Torvalds wrote:
> 
> 
> On Mon, 10 Nov 2008, Jens Axboe wrote:
> > > 
> > > Or we could just change the blk-tag.c logic to stop of
> > > find_first_zero_bit() returns >= some_value instead of starting at an
> > > offset? You don't need any extra locking for that.
> > 
> > Something like the below.
> 
> No, there were two reasons for doing it the way I did it, and this shows 
> both. One trivial, one subtle.
> 
> > +	if (!rq_is_sync(rq))
> > +		max_depth = 3 * max_depth / 4;
> 
> The trivial one here is that you round down. Imagine what happens if 
> "max_depth" was 1.
> 
> The subtler one was that the 'use starting offset' means that async and 
> sync can _share_ the tagspace, and while you limit async ones to a maximum 
> outstanding number, you really cut down on them only when sync ones really 
> have filled everything up.
> 
> In contrast, limiting like the above means that it's much easier to be in 
> the situation where you still have tags to use, but you've used them all 
> for reads, and you refuse to start a single write.

Good point. I'll do a counting solution for this instead.

> Anyway, I'll do the revert, since -rc4 is too late to discuss these 
> issues. I think we can easily re-do things when everybody is ok with the 
> code.

OK, we'll get it into shape for 2.6.29 instead.

-- 
Jens Axboe

Re: request to revert libata-convert-to-block-tagging patches

[Tejun Heo]

Jens Axboe wrote:
> On Mon, Nov 10 2008, Tejun Heo wrote:
>> Hello, all.
>>
>> I went through libata-convert-to-block-tagging today and found several
>> issues.
>>
>> 1. libata internal data structure for command context (qc) allocation is
>> bound to tag allocation, which means that block layer tagging should be
>> enabled for all controllers which have can_queue > 1.
> 
> Naturally, is there a problem there?

Queueing wasn't enabled for ATAPI device behind PMP so it made those
devices reuse already allocated qc's.  Not difficult to fix.

>> 2. blk-tag offsets allocation for non-sync requests.  I'm not confident
>> this is safe.  Till now, if there was only single command in flight for
>> the port, it was guaranteed that the qc gets tag zero whether the device
>> is NCQ capable or not.  qc allocation is tied tightly with hardware
>> command slot allocation and I don't think it's wise to change this
>> assumption.
>>
>> #1 is easy to fix but #2 requires either adding a spinlock or two atomic
>> variables to struct blk_queue_tag to keep the current behavior while
>> guaranteeing that tags are used in order.  Also, there's delay between
>> libata marks a request complete and the request actually gets completed
>> and the tag is freed.  If another request gets issued inbetween, the tag
>> number can't be guaranteed.  This can be worked around by re-mapping tag
>> number in libata depending on command type but, well then, it's worse
>> than the original implementation.
> 
> Or we could just change the blk-tag.c logic to stop of
> find_first_zero_bit() returns >= some_value instead of starting at an
> offset? You don't need any extra locking for that.

I tried that but there's a behavior difference.  If you reserve from
the beginning, the sync IOs prefer the reserved slots.  If you
reserved from the end, the sync IO prefer non-reserved slots.
ie. When 4 slots are reserved for sync IO, and 4 sync IOs are already
in flight, the fifth sync IO competes with async IOs on the same
ground in the former case but in the latter it either wins or is very
likely to take another reserved slot.

> The second part is more tricky I think, but I'm not sure there's a race
> there. For normally issued IO, queueing is restarted when the tag
> completes. There's a small softirq delay there, but that delay is before
> the tag is completed and queueing restarted. Any non-ncq command (eg
> through an ioctl) will have to wait for completion as well.

For drivers with .can_queue == 1, nothing can go wrong.  I was worried
about NCQ -> non-NCQ transition because blk-tag doesn't know that the
non-NCQ command is not to be scheduled with NCQ commands and will
happily assign any tag and in this case the race window definitely is
there.

>> So, please revert the following commits.
>>
>> 43a49cbdf31e812c0d8f553d433b09b421f5d52c
>> e013e13bf605b9e6b702adffbe2853cfc60e7806
>> 2fca5ccf97d2c28bcfce44f5b07d85e74e3cd18e
> 
> It's not a big deal to me, it can wait for 2.6.29 if there really are
> more issues there. But I'm not sure that your points are very valid, so
> lets please discuss it a bit more :-)

Yeap, I fully agree moving to blk tagging is good.  If we fix the
problem from #1, it's probably gonna be okay for most cases too.  I'm
just a little bit nervous because libata always has had this tag 0 for
non-NCQ commands assumption and this conversion changes that, so I was
hoping to update blk-tag such that such assumption can be guaranteed
first and then convert libata to be on the safe side.  Some
controllers use completely different command mechanism for different
protocols and it's much safer and more deterministic if same tag can
be guaranteed.

Thanks.

-- 
tejun

Re: request to revert libata-convert-to-block-tagging patches

[Linus Torvalds]

On Mon, 10 Nov 2008, Tejun Heo wrote:
>
> I'm just a little bit nervous because libata always has had this tag 0 
> for non-NCQ commands assumption and this conversion changes that, so I 
> was hoping to update blk-tag such that such assumption can be guaranteed 
> first and then convert libata to be on the safe side.  Some controllers 
> use completely different command mechanism for different protocols and 
> it's much safer and more deterministic if same tag can be guaranteed.

Yeah, I think that's a good argument. Even when controllers expect tags, 
it's certainyl quite possible that all they've ever been tested with have 
always started tag allocation from zero, so while the "start at an offset" 
thing is fairly clever for other reasons, it probably was the wrong thing 
to do.

Maybe we can just have something count "outstanding async/sync requests".

			Linus

Re: request to revert libata-convert-to-block-tagging patches

[Jeff Garzik]

Tejun Heo wrote:
> Hello, all.
> 
> I went through libata-convert-to-block-tagging today and found several
> issues.
> 
> 1. libata internal data structure for command context (qc) allocation is
> bound to tag allocation, which means that block layer tagging should be
> enabled for all controllers which have can_queue > 1.
> 
> 2. blk-tag offsets allocation for non-sync requests.  I'm not confident
> this is safe.  Till now, if there was only single command in flight for
> the port, it was guaranteed that the qc gets tag zero whether the device
> is NCQ capable or not.  qc allocation is tied tightly with hardware
> command slot allocation and I don't think it's wise to change this
> assumption.
> 
> #1 is easy to fix but #2 requires either adding a spinlock or two atomic
> variables to struct blk_queue_tag to keep the current behavior while
> guaranteeing that tags are used in order.  Also, there's delay between
> libata marks a request complete and the request actually gets completed
> and the tag is freed.  If another request gets issued inbetween, the tag
> number can't be guaranteed.  This can be worked around by re-mapping tag
> number in libata depending on command type but, well then, it's worse
> than the original implementation.
> 
> So, please revert the following commits.
> 
> 43a49cbdf31e812c0d8f553d433b09b421f5d52c
> e013e13bf605b9e6b702adffbe2853cfc60e7806
> 2fca5ccf97d2c28bcfce44f5b07d85e74e3cd18e

A bit late, since they're already in, but, ACK.  (I'm on East Coast 
Vampire time, apparently)

Now that this is resolved, please allow me a bit of grumbling.  I always 
thought the original course -- 2.6.29 -- was best for these patches.  I 
had even queued them for 2.6.29, when they found their way into 2.6.28 
anyway.  Without /any/ testing by libata maintainers or linux-next. 
Without even being tested on non-NCQ setups, apparently.

The process broke down completely with this patchset :(

I still want to see this stuff in 2.6.29 though; it is the right way to 
go: following the theme of using block rather than SCSI bits in libata 
generic code [when those bits are, themselves, generic rather than 
SCSI-specific].

	Jeff

Re: request to revert libata-convert-to-block-tagging patches

[Linus Torvalds]

On Mon, 10 Nov 2008, Jeff Garzik wrote:
> 
> Now that this is resolved, please allow me a bit of grumbling.  I always
> thought the original course -- 2.6.29 -- was best for these patches.

You can blame me on that one. I actually pushed for these, since I was 
interested in seeing what the impact of it all was. My bad.

			Linus