Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1362341 > unrolled thread

[PATCH 7/8] block: drbd: avoid to use BIO_MAX_SIZE

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 7/8] block: drbd: avoid to use BIO_MAX_SIZE Ming Lei <ming.lei@canonical.com> - 2016-03-22 07:20 +0100
    Re: [PATCH 7/8] block: drbd: avoid to use BIO_MAX_SIZE Christoph Hellwig <hch@infradead.org> - 2016-03-29 09:40 +0200
      Re: [PATCH 7/8] block: drbd: avoid to use BIO_MAX_SIZE Lars Ellenberg <lars.ellenberg@linbit.com> - 2016-03-29 10:20 +0200

#1362341 — [PATCH 7/8] block: drbd: avoid to use BIO_MAX_SIZE

drbd is the only user of BIO_MAX_SIZE, so use BIO_MAX_PAGES
instead.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/block/drbd/drbd_int.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
index c227fd4..10bfff1 100644
--- a/drivers/block/drbd/drbd_int.h
+++ b/drivers/block/drbd/drbd_int.h
@@ -1327,14 +1327,14 @@ struct bm_extent {
 #endif
 #endif
 
-/* BIO_MAX_SIZE is 256 * PAGE_CACHE_SIZE,
+/* Estimate max bio size as 256 * PAGE_CACHE_SIZE,
  * so for typical PAGE_CACHE_SIZE of 4k, that is (1<<20) Byte.
  * Since we may live in a mixed-platform cluster,
  * we limit us to a platform agnostic constant here for now.
  * A followup commit may allow even bigger BIO sizes,
  * once we thought that through. */
 #define DRBD_MAX_BIO_SIZE (1U << 20)
-#if DRBD_MAX_BIO_SIZE > BIO_MAX_SIZE
+#if DRBD_MAX_BIO_SIZE > (BIO_MAX_PAGES << PAGE_CACHE_SHIFT)
 #error Architecture not supported: DRBD_MAX_BIO_SIZE > BIO_MAX_SIZE
 #endif
 #define DRBD_MAX_BIO_SIZE_SAFE (1U << 12)       /* Works always = 4k */
-- 
1.9.1

[toc] | [next] | [standalone]

#1365948

On Tue, Mar 22, 2016 at 02:12:28PM +0800, Ming Lei wrote:
> drbd is the only user of BIO_MAX_SIZE, so use BIO_MAX_PAGES
> instead.

That whole code block looks completely bogus to me, although your patch
doesn't make it any worse.

I/O size for a network protocol shouldn't dependend on the number of
vectors in a kernel internal structure.

Well, getting rid of BIO_MAX_SIZE is worth it, so:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1365985

On Tue, Mar 29, 2016 at 12:31:24AM -0700, Christoph Hellwig wrote:
> On Tue, Mar 22, 2016 at 02:12:28PM +0800, Ming Lei wrote:
> > drbd is the only user of BIO_MAX_SIZE, so use BIO_MAX_PAGES
> > instead.
> 
> That whole code block looks completely bogus to me, although your patch
> doesn't make it any worse.
> 
> I/O size for a network protocol shouldn't dependend on the number of
> vectors in a kernel internal structure.

That's correct.  But we needed some limit there.
Initially, up until I changed it like six years ago iirc,
the receiving side would receive into a single bio.
So limiting us to what a single bio could usually handle
seemed like a good idea at the time.

Today, we should be able to handle 128 MiB easily,
maybe more. But that would require a protocol bump
to stay backwards compatible.

The part about "architecture not supported",
if our limit (1 MiB) is bigger than the "system" limit:
Never met that in real life. Probably not even possible.
Just a paranoia on my side: what if.
If that would have happened somewhere,
on some strange architecture or configuration,
I wanted to know about that.
Best way: don't even compile.

> Well, getting rid of BIO_MAX_SIZE is worth it, so:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks,

    Lars Ellenberg

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web