Prev: [PATCH 5/5] staging: slicoss: error handling with goto
Next: scsi: remove dma_is_consistent usage in 53c700
From: Mike Snitzer on 27 Jun 2010 10:10
On Sun, Jun 27 2010 at 5:39am -0400,
Christoph Hellwig <hch(a)lst.de> wrote:

> On Sat, Jun 26, 2010 at 03:56:51PM -0400, Mike Snitzer wrote:
> > Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so
> > means bio_has_data() will not be true until the SCSI layer adds the
> > payload to the discard request via blk_add_request_payload.
> >
> > bio_{enable,disable}_inline_vecs are not expected to be widely used so
> > they were exported using EXPORT_SYMBOL_GPL.
>
> Why do we need them exported at all?

Hmm, good point!

> Also some comments on these functions would be useful.

OK.

> Otherwise it looks good to me.

Thanks, I'll get a v2 of this patch out.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: FUJITA Tomonori on 28 Jun 2010 06:40
On Sat, 26 Jun 2010 15:56:51 -0400
Mike Snitzer <snitzer(a)redhat.com> wrote:

> Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so
> means bio_has_data() will not be true until the SCSI layer adds the
> payload to the discard request via blk_add_request_payload.
>
> bio_{enable,disable}_inline_vecs are not expected to be widely used so
> they were exported using EXPORT_SYMBOL_GPL.
>
> This patch avoids the need for the following VM accounting fix for
> discards: http://lkml.org/lkml/2010/6/23/361

Why do we need to avoid the above fix? Surely, the above fix is hacky
but much simpler than this patch.


> NOTE: Jens, you said you applied Tao Ma's fix but I cannot see it in
> your linux-2.6-block's for-next or for-2.6.36... as such I didn't revert
> it in this patch.
>
> Signed-off-by: Mike Snitzer <snitzer(a)redhat.com>
> ---
> block/blk-core.c | 2 ++
> block/blk-lib.c | 2 +-
> fs/bio.c | 19 +++++++++++++++++--
> include/linux/bio.h | 3 +++
> 4 files changed, 23 insertions(+), 3 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jens Axboe on 28 Jun 2010 08:40
On 2010-06-26 21:56, Mike Snitzer wrote:
> Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so
> means bio_has_data() will not be true until the SCSI layer adds the
> payload to the discard request via blk_add_request_payload.

Sorry, this looks horrible. What is the point of this?!

> bio_{enable,disable}_inline_vecs are not expected to be widely used so
> they were exported using EXPORT_SYMBOL_GPL.

Never export anything that doesn't have an in-kernel modular user.

> This patch avoids the need for the following VM accounting fix for
> discards: http://lkml.org/lkml/2010/6/23/361
> NOTE: Jens, you said you applied Tao Ma's fix but I cannot see it in
> your linux-2.6-block's for-next or for-2.6.36... as such I didn't revert
> it in this patch.

It's in the for-linus branch, that is stuff headed for the current
kernel.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mike Snitzer on 28 Jun 2010 08:40
On Mon, Jun 28 2010 at 8:34am -0400,
Jens Axboe <axboe(a)kernel.dk> wrote:

> On 2010-06-26 21:56, Mike Snitzer wrote:
> > Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so
> > means bio_has_data() will not be true until the SCSI layer adds the
> > payload to the discard request via blk_add_request_payload.
>
> Sorry, this looks horrible.

Your judgment isn't giving me much to work with... not sure where I go
with "horrible".

> What is the point of this?!

Enables discard requests with _not_ return true for bio_has_data().

> > bio_{enable,disable}_inline_vecs are not expected to be widely used so
> > they were exported using EXPORT_SYMBOL_GPL.
>
> Never export anything that doesn't have an in-kernel modular user.

Yeap, v2 removed the exports.

> > This patch avoids the need for the following VM accounting fix for
> > discards: http://lkml.org/lkml/2010/6/23/361
> > NOTE: Jens, you said you applied Tao Ma's fix but I cannot see it in
> > your linux-2.6-block's for-next or for-2.6.36... as such I didn't revert
> > it in this patch.
>
> It's in the for-linus branch, that is stuff headed for the current
> kernel.

OK.

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mike Snitzer on 28 Jun 2010 08:40
On Mon, Jun 28 2010 at 6:33am -0400,
FUJITA Tomonori <fujita.tomonori(a)lab.ntt.co.jp> wrote:

> On Sat, 26 Jun 2010 15:56:51 -0400
> Mike Snitzer <snitzer(a)redhat.com> wrote:
>
> > Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so
> > means bio_has_data() will not be true until the SCSI layer adds the
> > payload to the discard request via blk_add_request_payload.
> >
> > bio_{enable,disable}_inline_vecs are not expected to be widely used so
> > they were exported using EXPORT_SYMBOL_GPL.
> >
> > This patch avoids the need for the following VM accounting fix for
> > discards: http://lkml.org/lkml/2010/6/23/361
>
> Why do we need to avoid the above fix?

We don't _need_ to. We avoid the need for it as a side-effect of the
cleanup that my patch provides.

> Surely, the above fix is hacky but much simpler than this patch.

My patch wasn't meant as an alternative to Tao Ma's patch. Again, it
just obviates the need for it.

Your tolerance for "hacky" is difficult to understand. On the one-hand
(PATCH 1/2) you have no tolerance for "hacky" fixes for leaks (that
introduce a short-term SCSI layering violation). But in this case
you're perfectly fine with BIO_RW_DISCARD special casing?

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH 5/5] staging: slicoss: error handling with goto
Next: scsi: remove dma_is_consistent usage in 53c700