Re: [patch 1/2]block: handle merged discard request

Re: [patch 1/2]block: handle merged discard request

[Mike Snitzer]

On Thu, Jun 21, 2012 at 11:46 PM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:
>
>>> There are several additional commands in the pipeline where the 1:1
>>> mapping between DMA size and block range is invalid. I want to get
>>> rid of the 1:1 assumption in general so we can handle any command
>>> without these evil workarounds.
>
> Christoph> What's the progress on getting these issues sorted out?
>
> This has bitrotted for a while. I'll put it on my list. I should finally
> have some bandwidth again next week...

Hey Martin,

I rebased (and fixed/tested) your writesame patches on v3.6-rc2 +
jens' for-linus branch, the git tree is available here:
https://github.com/snitm/linux/tree/writesame

I've also made the updated patchset available here:
http://people.redhat.com/msnitzer/patches/upstream/writesame/series.html

Should the writesame patches come before any discard merge or 1:1 DMA
and block range assumption fixes?
NOTE (for others besides martin):
http://people.redhat.com/msnitzer/patches/upstream/writesame/0001-block-Clean-up-merge-logic.patch
removes all the discard merge hacks; I think it provides a clean
baseline to then layer discard merge support back in -- but maybe
that's a flawed strategy?

Could be I've wasted a few hours by rebasing these patches...
regardless, it would be great if you could share what your plans are.

Thanks!

Re: [patch 1/2]block: handle merged discard request

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike> Could be I've wasted a few hours by rebasing these patches...
Mike> regardless, it would be great if you could share what your plans
Mike> are.

Heh, I worked on syncing my patch queue up to Jens' and James' trees
this afternoon. But I didn't quite finish the block stuff, mainly due to
some conflicts with a few topology changes I also have pending.

I'll take a look at your series. Maybe I'll swap things around and put
the topology changes on top instead of below. Leverage some of the work
you did...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [patch 1/2]block: handle merged discard request

[Mike Snitzer]

On Fri, Aug 17 2012 at 11:47pm -0400,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
> 
> Mike> Could be I've wasted a few hours by rebasing these patches...
> Mike> regardless, it would be great if you could share what your plans
> Mike> are.
> 
> Heh, I worked on syncing my patch queue up to Jens' and James' trees
> this afternoon. But I didn't quite finish the block stuff, mainly due to
> some conflicts with a few topology changes I also have pending.
> 
> I'll take a look at your series. Maybe I'll swap things around and put
> the topology changes on top instead of below. Leverage some of the work
> you did...

OK, just FYI, I had to change bio_has_data() to test bio->bi_vcnt
(rather than bio->bi_io_vec != NULL) because a discard bio has a
non-NULL bio->bi_io_vec (likely points to the bio->bi_inline_vecs but I
didn't check yet).

But I haven't put my finger on _why_ a discard bio has bio->bi_io_vec
(but given my use of DM, bio comes from bio_alloc_bioset, and DM passes
original bio->bi_max_vecs for nr_iovecs).

Anyway, this bio_has_data() change seemed reasonable considering
bio_data() checks bio->bi_vcnt.

Re: [patch 1/2]block: handle merged discard request

[Christoph Hellwig]

On Mon, Aug 20, 2012 at 09:57:39AM -0400, Mike Snitzer wrote:
> But I haven't put my finger on _why_ a discard bio has bio->bi_io_vec
> (but given my use of DM, bio comes from bio_alloc_bioset, and DM passes
> original bio->bi_max_vecs for nr_iovecs).

TRIM has a payload and we cheay by preallocation a data page for it.

Re: [patch 1/2]block: handle merged discard request

[Mike Snitzer]

On Mon, Aug 20 2012 at  9:58am -0400,
Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Aug 20, 2012 at 09:57:39AM -0400, Mike Snitzer wrote:
> > But I haven't put my finger on _why_ a discard bio has bio->bi_io_vec
> > (but given my use of DM, bio comes from bio_alloc_bioset, and DM passes
> > original bio->bi_max_vecs for nr_iovecs).
> 
> TRIM has a payload and we cheay by preallocation a data page for it.

Thought we pushed that down?  Hence sd_setup_discard_cmnd's
alloc_page + blk_add_request_payload hack.

Re: [patch 1/2]block: handle merged discard request

[Christoph Hellwig]

On Mon, Aug 20, 2012 at 10:12:29AM -0400, Mike Snitzer wrote:
> Thought we pushed that down?  Hence sd_setup_discard_cmnd's
> alloc_page + blk_add_request_payload hack.

Yeah, but we still need the bio_vec from early on, as it's allocated as
part of the bio.