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

This is a discussion on request to revert libata-convert-to-block-tagging patches - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 11 of 11

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

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    Cc: Jens Axobe
    Cc: Jeff Garzik
    ---
    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 */

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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



    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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



    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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



    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread