| Message ID | 20170316161235.27110-3-tom.leiming@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <tom.leiming@gmail.com> > --- > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 24, 2017 at 1:57 PM, NeilBrown <neilb@suse.com> 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
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
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?
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
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 */
Both raid1 and raid10 share common resync block size and page count, so move them into md.h. Signed-off-by: Ming Lei <tom.leiming@gmail.com> --- drivers/md/md.h | 5 +++++ drivers/md/raid1.c | 2 -- drivers/md/raid10.c | 3 --- 3 files changed, 5 insertions(+), 5 deletions(-)