Re: [PATCH v3 02/14] md: move two macros into md.h

[NeilBrown]

On Mon, Mar 27 2017, Christoph Hellwig wrote:

> On Fri, Mar 24, 2017 at 09:53:25AM -0700, Shaohua Li wrote:
>> 
>> I had the same concern when I looked at this patch firstly. The number for
>> raid1/10 doesn't need to be the same. But if we don't move the number to a
>> generic header, the third patch will become a little more complicated. I
>> eventually ignored this issue. If we really need different number for 
>> raid1/10,
>> lets do it at that time.
>
> Which brings up my usual queastion:  Is is really that benefitical for
> us to keep the raid1.c code around instead of making it a special short
> cut case in raid10.c?

Patches welcome.

They would need to handle write-mostly and write-behind.  They would
also need to avoid the assumption of a chunk size for RAID1.
Undoubtedly do-able.  Hard to say how beneficial it would be, or how much
it would cost.

NeilBrown


signature.asc
Description: PGP signature

Re: [PATCH v3 02/14] md: move two macros into md.h

[Christoph Hellwig]

On Fri, Mar 24, 2017 at 09:53:25AM -0700, Shaohua Li wrote:
> 
> I had the same concern when I looked at this patch firstly. The number for
> raid1/10 doesn't need to be the same. But if we don't move the number to a
> generic header, the third patch will become a little more complicated. I
> eventually ignored this issue. If we really need different number for 
> raid1/10,
> lets do it at that time.

Which brings up my usual queastion:  Is is really that benefitical for
us to keep the raid1.c code around instead of making it a special short
cut case in raid10.c?

Re: [PATCH v3 02/14] md: move two macros into md.h

[Shaohua Li]

On Fri, Mar 24, 2017 at 04:57:37PM +1100, Neil Brown wrote:
> On Fri, Mar 17 2017, Ming Lei wrote:
> 
> > Both raid1 and raid10 share common resync
> > block size and page count, so move them into md.h.
> 
> I don't think this is necessary.
> These are just "magic" numbers.  They don't have any real
> meaning and so don't belong in md.h, or and .h file.
> 
> Possibly we should find more meaningful numbers, or make them auto-size
> or something.  I'm also happy for them to stay as they are for now.
> But I don't think we should pretend that they are meaningful.

I had the same concern when I looked at this patch firstly. The number for
raid1/10 doesn't need to be the same. But if we don't move the number to a
generic header, the third patch will become a little more complicated. I
eventually ignored this issue. If we really need different number for raid1/10,
lets do it at that time.

I think your suggestion that moving the number to raid1-10.h makes sense, and
add a comment declaring the number isn't required to be the same for raid1/10.

Thanks,
Shaohua

Re: [PATCH v3 02/14] md: move two macros into md.h

[Ming Lei]

On Fri, Mar 24, 2017 at 1:57 PM, NeilBrown  wrote:
> On Fri, Mar 17 2017, Ming Lei wrote:
>
>> Both raid1 and raid10 share common resync
>> block size and page count, so move them into md.h.
>
> I don't think this is necessary.
> These are just "magic" numbers.  They don't have any real
> meaning and so don't belong in md.h, or and .h file.

The thing is that RESYNC_PAGES is needed in the following patch 3:

 [PATCH v3 03/14] md: prepare for managing resync I/O pages in clean way

so how about moving the macros into raid1-10.h? Cause you suggest
to create that header for holding the introduced helpers in patch3.

Thanks,
Ming

Re: [PATCH v3 02/14] md: move two macros into md.h

[NeilBrown]

On Fri, Mar 17 2017, Ming Lei wrote:

> Both raid1 and raid10 share common resync
> block size and page count, so move them into md.h.

I don't think this is necessary.
These are just "magic" numbers.  They don't have any real
meaning and so don't belong in md.h, or and .h file.

Possibly we should find more meaningful numbers, or make them auto-size
or something.  I'm also happy for them to stay as they are for now.
But I don't think we should pretend that they are meaningful.

Thanks,
NeilBrown


>
> Signed-off-by: Ming Lei 
> ---
>  drivers/md/md.h | 5 +
>  drivers/md/raid1.c  | 2 --
>  drivers/md/raid10.c | 3 ---
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index b8859cbf84b6..1d63239a1be4 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -715,4 +715,9 @@ static inline void mddev_check_writesame(struct mddev 
> *mddev, struct bio *bio)
>   !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors)
>   mddev->queue->limits.max_write_same_sectors = 0;
>  }
> +
> +/* Maximum size of each resync request */
> +#define RESYNC_BLOCK_SIZE (64*1024)
> +#define RESYNC_PAGES ((RESYNC_BLOCK_SIZE + PAGE_SIZE-1) / PAGE_SIZE)
> +
>  #endif /* _MD_MD_H */
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4a0b2ad5025e..908e2caeb704 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -94,10 +94,8 @@ static void r1bio_pool_free(void *r1_bio, void *data)
>   kfree(r1_bio);
>  }
>  
> -#define RESYNC_BLOCK_SIZE (64*1024)
>  #define RESYNC_DEPTH 32
>  #define RESYNC_SECTORS (RESYNC_BLOCK_SIZE >> 9)
> -#define RESYNC_PAGES ((RESYNC_BLOCK_SIZE + PAGE_SIZE-1) / PAGE_SIZE)
>  #define RESYNC_WINDOW (RESYNC_BLOCK_SIZE * RESYNC_DEPTH)
>  #define RESYNC_WINDOW_SECTORS (RESYNC_WINDOW >> 9)
>  #define CLUSTER_RESYNC_WINDOW (16 * RESYNC_WINDOW)
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index b4a56a488668..2b40420299e3 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -125,9 +125,6 @@ static void r10bio_pool_free(void *r10_bio, void *data)
>   kfree(r10_bio);
>  }
>  
> -/* Maximum size of each resync request */
> -#define RESYNC_BLOCK_SIZE (64*1024)
> -#define RESYNC_PAGES ((RESYNC_BLOCK_SIZE + PAGE_SIZE-1) / PAGE_SIZE)
>  /* amount of memory to reserve for resync requests */
>  #define RESYNC_WINDOW (1024*1024)
>  /* maximum number of concurrent requests, memory permitting */
> -- 
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature