Re: [PATCH 5/5] ide-tape: remove the last remains of pipelining

[Bartlomiej Zolnierkiewicz]

On Saturday 29 March 2008, Borislav Petkov wrote:
> This patch converts the tape->merge_stage pipeline stage into tape->bh, a singly
> linked list of idetape_bh's, each of which is a tag attached to one or more pages
> serving as a data buffer for chrdev requests. In particular,
> 
> 1. makes tape->bh the data buffer of size tape->buffer_size which is computed
> from the Continuous Transfer Limit value in the caps page and the tape block
> size. The chrdev rw routines use this buffer as an intermediary location to
> shuffle data to and from.

ide-tape supports write buffering (if number of bytes written to character
device is < tape->buffer_size) and it seems to be the main reason behind
tape->merge_stage.  It seems that this feature gets killed by the above
change (we may actually want to kill it later in order to implement direct
mapping of user pages but this shouldn't be done unless really necessary
since it may negatively affect performance of some "poorly" written apps).

Also:
tape->bh serves as the current bh pointer in idetape_chrdev_{read,write}()
so it doesn't seem like we can mix it with merge_stage->bh (OTOH we should
be fine with having tape->merge_bh).

> 2. mv tape->merge_stage_size => tape->cur_buf_size as it contains the offset
> within tape->bh
> 
> 3. get rid of pipeline stage idetape_stage_t, tape->merge_stage
> and pipeline-related functions __idetape_discard_read_pipeline(),
> idetape_discard_read_pipeline(), idetape_empty_write_pipeline()

Existing tape->merge_stage is first taken care of at the beggining of
idetape_chrdev_{read,write}() and only then the new one is allocated,
the above functions play important part in handling write buffering
(which is _independent_ from pipelined-mode) and idetape_discard...()
seems to play some part in tape positioning for some ioctls - I worry
that they can't be just simply deleted.

> 4. code chunk "if (tape->merge_stage_size) {...}" in idetape_chrdev_read() is not
> needed since tape->merge_stage_size, tape->cur_buf_size resp., is zeroed out in
> idetape_init_read() couple of lines above

Unless the previous user request was also idetape_chrdev_read() since in this
case idetape_init_read() returns early and existing ->merge_stage is re-used.

[...]

I like very much the general direction that this patch is going but the above
details need to be taken care of.  I suggest splitting the patch on many
smaller ones (i.e. starting with 'tape->merge_stage -> tape->merge_bh', then
inlining __idetape_discard_read_pipeline() etc.) so we may closely review
such fine-grained and _obvious_ steps.

[ ide-tape got _much_ better after your rewrites but some old parts are still
  quite puzzling & mined with gotchas - we shouldn't let guard down yet. ;) ]

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