* Re: Linear device of two arrays
From: Veljko @ 2017-07-12 10:21 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <e394625b-18d7-d141-3957-3c16c9bc6e44@gmail.com>
Hello Neil,
On 07/10/2017 01:03 PM, Veljko wrote:
> On 07/10/2017 12:37 AM, NeilBrown wrote:
>> I wasn't clear to me that I needed to chime in.. and the complete lack
>> of details (not even an "mdadm --examine" output), meant I could only
>> answer in vague generalizations.
>> However, seeing you asked.
>> If you really want to have a 'linear' of 2 RAID10s, then
>> 0/ unmount the xfs filesystem
>> 1/ backup the last few megabytes of the device
>> dd if=/dev/mdXX of=/safe/place/backup bs=1M skip=$BIGNUM
>> 2/ create a linear array of the two RAID10s, ensuring the
>> metadata is v1.0, and the dataoffset is zero (should be default with
>> 1.0)
>> mdadm -C /dev/mdZZ -l linear -n 2 -e 1.0 --data-offset=0 /dev/mdXX
>> /dev/mdYY
>> 3/ restore the saved data
>> dd of=/dev/mdZZ if=/safe/place/backup bs=1M seek=$BIGNUM
>> 4/ grow the xfs filesystem
>> 5/ be happy.
>>
>> I cannot comment on the values of "few" and "$BUGNUM" without seeing
>> specifics.
>>
>> NeilBrown
>
> Thanks for your response, Neil!
>
> md0 is boot (raid1), md1 is root (raid10) and md2 is data (raid10) that
> I need to expand. Here are details:
>
>
> # mdadm --detail /dev/md0
> /dev/md0:
> Version : 1.2
> Creation Time : Mon Sep 10 14:45:11 2012
> Raid Level : raid1
> Array Size : 488128 (476.77 MiB 499.84 MB)
> Used Dev Size : 488128 (476.77 MiB 499.84 MB)
> Raid Devices : 2
> Total Devices : 2
> Persistence : Superblock is persistent
>
> Update Time : Mon Jul 3 11:57:24 2017
> State : clean
> Active Devices : 2
> Working Devices : 2
> Failed Devices : 0
> Spare Devices : 0
>
> Name : backup1:0 (local to host backup1)
> UUID : e5a17766:b4df544d:c2770d6e:214113ec
> Events : 302
>
> Number Major Minor RaidDevice State
> 2 8 18 0 active sync /dev/sdb2
> 3 8 34 1 active sync /dev/sdc2
>
>
> # mdadm --detail /dev/md1
> /dev/md1:
> Version : 1.2
> Creation Time : Fri Sep 14 12:39:00 2012
> Raid Level : raid10
> Array Size : 97590272 (93.07 GiB 99.93 GB)
> Used Dev Size : 48795136 (46.53 GiB 49.97 GB)
> Raid Devices : 4
> Total Devices : 4
> Persistence : Superblock is persistent
>
> Update Time : Mon Jul 10 12:30:46 2017
> State : clean
> Active Devices : 4
> Working Devices : 4
> Failed Devices : 0
> Spare Devices : 0
>
> Layout : near=2
> Chunk Size : 512K
>
> Name : backup1:1 (local to host backup1)
> UUID : 91560d5a:245bbc56:cc08b0ce:9c78fea1
> Events : 1003350
>
> Number Major Minor RaidDevice State
> 4 8 19 0 active sync set-A /dev/sdb3
> 6 8 35 1 active sync set-B /dev/sdc3
> 7 8 50 2 active sync set-A /dev/sdd2
> 5 8 2 3 active sync set-B /dev/sda2
>
>
> # mdadm --detail /dev/md2
> /dev/md2:
> Version : 1.2
> Creation Time : Fri Sep 14 12:40:13 2012
> Raid Level : raid10
> Array Size : 5761631232 (5494.72 GiB 5899.91 GB)
> Used Dev Size : 2880815616 (2747.36 GiB 2949.96 GB)
> Raid Devices : 4
> Total Devices : 4
> Persistence : Superblock is persistent
>
> Update Time : Mon Jul 10 12:32:51 2017
> State : clean
> Active Devices : 4
> Working Devices : 4
> Failed Devices : 0
> Spare Devices : 0
>
> Layout : near=2
> Chunk Size : 512K
>
> Name : backup1:2 (local to host backup1)
> UUID : f6eeaa57:a55f36ff:6980a62a:d4781e44
> Events : 2689040
>
> Number Major Minor RaidDevice State
> 4 8 20 0 active sync set-A /dev/sdb4
> 6 8 36 1 active sync set-B /dev/sdc4
> 7 8 51 2 active sync set-A /dev/sdd3
> 5 8 3 3 active sync set-B /dev/sda3
>
>
> And here is examine output for md2 partitions:
>
> # mdadm --examine /dev/sda3
> /dev/sda3:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x0
> Array UUID : f6eeaa57:a55f36ff:6980a62a:d4781e44
> Name : backup1:2 (local to host backup1)
> Creation Time : Fri Sep 14 12:40:13 2012
> Raid Level : raid10
> Raid Devices : 4
>
> Avail Dev Size : 5762609152 (2747.83 GiB 2950.46 GB)
> Array Size : 5761631232 (5494.72 GiB 5899.91 GB)
> Used Dev Size : 5761631232 (2747.36 GiB 2949.96 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=977920 sectors
> State : clean
> Device UUID : 92beeec2:7ff92b1d:473a9641:2a078b16
>
> Update Time : Mon Jul 10 12:35:53 2017
> Checksum : d1abfc30 - correct
> Events : 2689040
>
> Layout : near=2
> Chunk Size : 512K
>
> Device Role : Active device 3
> Array State : AAAA ('A' == active, '.' == missing, 'R' == replacing)
>
>
> # mdadm --examine /dev/sdb4
> /dev/sdb4:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x0
> Array UUID : f6eeaa57:a55f36ff:6980a62a:d4781e44
> Name : backup1:2 (local to host backup1)
> Creation Time : Fri Sep 14 12:40:13 2012
> Raid Level : raid10
> Raid Devices : 4
>
> Avail Dev Size : 5761632256 (2747.36 GiB 2949.96 GB)
> Array Size : 5761631232 (5494.72 GiB 5899.91 GB)
> Used Dev Size : 5761631232 (2747.36 GiB 2949.96 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1024 sectors
> State : clean
> Device UUID : 01e1cb21:01a011a9:85761911:9b4d437a
>
> Update Time : Mon Jul 10 12:37:00 2017
> Checksum : ef9b6012 - correct
> Events : 2689040
>
> Layout : near=2
> Chunk Size : 512K
>
> Device Role : Active device 0
> Array State : AAAA ('A' == active, '.' == missing, 'R' == replacing)
>
>
>
> # mdadm --examine /dev/sdc4
> /dev/sdc4:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x0
> Array UUID : f6eeaa57:a55f36ff:6980a62a:d4781e44
> Name : backup1:2 (local to host backup1)
> Creation Time : Fri Sep 14 12:40:13 2012
> Raid Level : raid10
> Raid Devices : 4
>
> Avail Dev Size : 5761632256 (2747.36 GiB 2949.96 GB)
> Array Size : 5761631232 (5494.72 GiB 5899.91 GB)
> Used Dev Size : 5761631232 (2747.36 GiB 2949.96 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1024 sectors
> State : clean
> Device UUID : 1a2c966f:a78ffaf3:83cf37d4:135087b7
>
> Update Time : Mon Jul 10 12:37:53 2017
> Checksum : 88b0f680 - correct
> Events : 2689040
>
> Layout : near=2
> Chunk Size : 512K
>
> Device Role : Active device 1
> Array State : AAAA ('A' == active, '.' == missing, 'R' == replacing)
>
>
>
>
> # mdadm --examine /dev/sdd3
> /dev/sdd3:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x0
> Array UUID : f6eeaa57:a55f36ff:6980a62a:d4781e44
> Name : backup1:2 (local to host backup1)
> Creation Time : Fri Sep 14 12:40:13 2012
> Raid Level : raid10
> Raid Devices : 4
>
> Avail Dev Size : 5762609152 (2747.83 GiB 2950.46 GB)
> Array Size : 5761631232 (5494.72 GiB 5899.91 GB)
> Used Dev Size : 5761631232 (2747.36 GiB 2949.96 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=977920 sectors
> State : clean
> Device UUID : 52f92e76:15228eee:a20c1ee5:8d4a17d2
>
> Update Time : Mon Jul 10 12:38:24 2017
> Checksum : b56275df - correct
> Events : 2689040
>
> Layout : near=2
> Chunk Size : 512K
>
> Device Role : Active device 2
> Array State : AAAA ('A' == active, '.' == missing, 'R' == replacing)
Do you know what would be the "few" and "$BIGNUM" values from the output
above?
Since I need to expand md2 device, I guess `that I need to subtract
"few" number of megabytes as ($few x 1024 x 1024) in bytes from array
size of md2 (in my case 5761631232). Is this correct? $BIGNUM is the
size of md2 array? How to know how many megabytes needs to be backed up?
Data offset is not zero on md2 partitions. Is that a dealbreaker?
Would it be than better to reshape the current RAID10 to increase the
number of devices used from 4 to 8 (as advised by Roman)?
Regards,
Veljko
^ permalink raw reply
* Interesting mount problem on openSUSE
From: Wols Lists @ 2017-07-12 13:59 UTC (permalink / raw)
To: linux-raid
Not my problem !!! Seen on the opensuse mailing list - a current thread...
Somebody has created a v1.0 mirror, which mounts fine on openSUSE 12.2,
but not Leap 42.2.
The two drives making up the mirror are sdb9 and sdc9, and the array is
called md9.
The filesystem on md9 is the same size as sdb9 or sdc9. The guess is the
array was created, and then the file system was created on one of the sd
devices.
The weirdo is why 12.2 mounts the partition fine, but 42.2 won't. Has
collision checking been tightened up? Might there be a change in the
superblock where the filesystem has overwritten it in a manner 42.2 can
detect but 12.2 can't?
As I say, it's not my problem, and it's clearly an incorrect
configuration, but it's weird why the older distro has no problem but
the newer one won't mount it.
Cheers,
Wol
^ permalink raw reply
* Re: [PATCH] raid5-ppl: use BIOSET_NEED_BVECS when creating bioset
From: Shaohua Li @ 2017-07-12 16:25 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: linux-raid
In-Reply-To: <20170711141624.27051-1-artur.paszkiewicz@intel.com>
On Tue, Jul 11, 2017 at 04:16:24PM +0200, Artur Paszkiewicz wrote:
> This bioset is used for allocating bios with nr_iovecs > 0 so this flag
> must be set.
>
> Fixes: 011067b05668 ("blk: replace bioset_create_nobvec() with a flags arg to bioset_create()")
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Applied, thanks!
> ---
> drivers/md/raid5-ppl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
> index 77cce3573aa8..44ad5baf3206 100644
> --- a/drivers/md/raid5-ppl.c
> +++ b/drivers/md/raid5-ppl.c
> @@ -1150,7 +1150,7 @@ int ppl_init_log(struct r5conf *conf)
> goto err;
> }
>
> - ppl_conf->bs = bioset_create(conf->raid_disks, 0, 0);
> + ppl_conf->bs = bioset_create(conf->raid_disks, 0, BIOSET_NEED_BVECS);
> if (!ppl_conf->bs) {
> ret = -ENOMEM;
> goto err;
> --
> 2.13.1
>
^ permalink raw reply
* Re: [PATCH v3 05/14] md: raid1: don't use bio's vec table to manage resync pages
From: Shaohua Li @ 2017-07-12 16:30 UTC (permalink / raw)
To: Ming Lei
Cc: NeilBrown, Ming Lei, Jens Axboe,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig
In-Reply-To: <CACVXFVOu92NhB+TmytW8Nk6bmL83cDHJFCf=gH5a_+GM6ov5kA@mail.gmail.com>
On Wed, Jul 12, 2017 at 09:40:10AM +0800, Ming Lei wrote:
> On Tue, Jul 11, 2017 at 7:14 AM, NeilBrown <neilb@suse.com> wrote:
> > On Mon, Jul 10 2017, Shaohua Li wrote:
> >
> >> On Mon, Jul 10, 2017 at 03:25:41PM +0800, Ming Lei wrote:
> >>> On Mon, Jul 10, 2017 at 02:38:19PM +1000, NeilBrown wrote:
> >>> > On Mon, Jul 10 2017, Ming Lei wrote:
> >>> >
> >>> > > On Mon, Jul 10, 2017 at 11:35:12AM +0800, Ming Lei wrote:
> >>> > >> On Mon, Jul 10, 2017 at 7:09 AM, NeilBrown <neilb@suse.com> wrote:
> >>> > ...
> >>> > >> >> +
> >>> > >> >> + rp->idx = 0;
> >>> > >> >
> >>> > >> > This is the only place the ->idx is initialized, in r1buf_pool_alloc().
> >>> > >> > The mempool alloc function is suppose to allocate memory, not initialize
> >>> > >> > it.
> >>> > >> >
> >>> > >> > If the mempool_alloc() call cannot allocate memory it will use memory
> >>> > >> > from the pool. If this memory has already been used, then it will no
> >>> > >> > longer have the initialized value.
> >>> > >> >
> >>> > >> > In short: you need to initialise memory *after* calling
> >>> > >> > mempool_alloc(), unless you ensure it is reset to the init values before
> >>> > >> > calling mempool_free().
> >>> > >> >
> >>> > >> > https://bugzilla.kernel.org/show_bug.cgi?id=196307
> >>> > >>
> >>> > >> OK, thanks for posting it out.
> >>> > >>
> >>> > >> Another fix might be to reinitialize the variable(rp->idx = 0) in
> >>> > >> r1buf_pool_free().
> >>> > >> Or just set it as zero every time when it is used.
> >>> > >>
> >>> > >> But I don't understand why mempool_free() calls pool->free() at the end of
> >>> > >> this function, which may cause to run pool->free() on a new allocated buf,
> >>> > >> seems a bug in mempool?
> >>> > >
> >>> > > Looks I missed the 'return' in mempool_free(), so it is fine.
> >>> > >
> >>> > > How about the following fix?
> >>> >
> >>> > It looks like it would probably work, but it is rather unusual to
> >>> > initialise something just before freeing it.
> >>> >
> >>> > Couldn't you just move the initialization to shortly after the
> >>> > mempool_alloc() call. There looks like a good place that already loops
> >>> > over all the bios....
> >>>
> >>> OK, follows the revised patch according to your suggestion.
> >
> > Thanks.
> >
> > That isn't as tidy as I hoped. So I went deeper into the code to try to
> > understand why...
> >
> > I think that maybe we should just discard the ->idx field completely.
> > It is only used in this code:
> >
> > do {
> > struct page *page;
> > int len = PAGE_SIZE;
> > if (sector_nr + (len>>9) > max_sector)
> > len = (max_sector - sector_nr) << 9;
> > if (len == 0)
> > break;
> > for (bio= biolist ; bio ; bio=bio->bi_next) {
> > struct resync_pages *rp = get_resync_pages(bio);
> > page = resync_fetch_page(rp, rp->idx++);
> > /*
> > * won't fail because the vec table is big enough
> > * to hold all these pages
> > */
> > bio_add_page(bio, page, len, 0);
> > }
> > nr_sectors += len>>9;
> > sector_nr += len>>9;
> > } while (get_resync_pages(biolist)->idx < RESYNC_PAGES);
> >
> > and all of the different 'rp' always have the same value for 'idx'.
> > This code is more complex than it needs to be. This is because it used
> > to be possible for bio_add_page() to fail. That cannot happen any more.
> > So we can make the code something like:
> >
> > for (idx = 0; idx < RESYNC_PAGES; idx++) {
> > struct page *page;
> > int len = PAGE_SIZE;
> > if (sector_nr + (len >> 9) > max_sector)
> > len = (max_sector - sector_nr) << 9
> > if (len == 0)
> > break;
> > for (bio = biolist; bio; bio = bio->bi_next) {
> > struct resync_pages *rp = get_resync_pages(bio);
> > page = resync_fetch_page(rp, idx);
> > bio_add_page(bio, page, len, 0);
> > }
> > nr_sectors += len >> 9;
> > sector_nr += len >> 9;
> > }
> >
> > Or did I miss something?
>
> I think this approach is much clean.
Thought I suggested not using the 'idx' in your previous post, but you said
there is reason (not because of bio_add_page) not to do it. Is that changed?
can't remember the details, I need to dig the mail archives.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 1/2] md: remove 'idx' from 'struct resync_pages'
From: NeilBrown @ 2017-07-12 23:58 UTC (permalink / raw)
To: Shaohua Li, linux-raid
Cc: linux-block, Jens Axboe, Christoph Hellwig, Ming Lei
In-Reply-To: <20170712082845.436-1-ming.lei@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4356 bytes --]
On Wed, Jul 12 2017, Ming Lei wrote:
> bio_add_page() won't fail for resync bio, and the page index for each
> bio is same, so remove it.
>
> More importantly the 'idx' of 'struct resync_pages' is initialized in
> mempool allocator function, this way is wrong since mempool is only
> responsible for allocation, we can't use that for initialization.
>
> Suggested-by: NeilBrown <neilb@suse.com>
> Reported-by: NeilBrown <neilb@suse.com>
> Fixes: f0250618361d(md: raid10: don't use bio's vec table to manage resync pages)
> Fixes: 98d30c5812c3(md: raid1: don't use bio's vec table to manage resync pages)
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> drivers/md/md.h | 1 -
> drivers/md/raid1.c | 6 +++---
> drivers/md/raid10.c | 6 +++---
> 3 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 991f0fe2dcc6..2c780aa8d07f 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -736,7 +736,6 @@ static inline void mddev_check_write_zeroes(struct mddev *mddev, struct bio *bio
>
> /* for managing resync I/O pages */
> struct resync_pages {
> - unsigned idx; /* for get/put page from the pool */
> void *raid_bio;
> struct page *pages[RESYNC_PAGES];
> };
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 3febfc8391fb..7901ddc3362f 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -170,7 +170,6 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
> resync_get_all_pages(rp);
> }
>
> - rp->idx = 0;
> rp->raid_bio = r1_bio;
> bio->bi_private = rp;
> }
> @@ -2619,6 +2618,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
> int good_sectors = RESYNC_SECTORS;
> int min_bad = 0; /* number of sectors that are bad in all devices */
> int idx = sector_to_idx(sector_nr);
> + int page_idx = 0;
>
> if (!conf->r1buf_pool)
> if (init_resync(conf))
> @@ -2846,7 +2846,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
> bio = r1_bio->bios[i];
> rp = get_resync_pages(bio);
> if (bio->bi_end_io) {
> - page = resync_fetch_page(rp, rp->idx++);
> + page = resync_fetch_page(rp, page_idx);
>
> /*
> * won't fail because the vec table is big
> @@ -2858,7 +2858,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
> nr_sectors += len>>9;
> sector_nr += len>>9;
> sync_blocks -= (len>>9);
> - } while (get_resync_pages(r1_bio->bios[disk]->bi_private)->idx < RESYNC_PAGES);
> + } while (page_idx++ < RESYNC_PAGES);
I think you want ++page_idx < RESYNC_PAGES, otherwise there will be
one pass through the loop where page_idx == RESYNC_PAGES
>
> r1_bio->sectors = nr_sectors;
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 5026e7ad51d3..e594ca610f27 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -221,7 +221,6 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
> resync_get_all_pages(rp);
> }
>
> - rp->idx = 0;
> rp->raid_bio = r10_bio;
> bio->bi_private = rp;
> if (rbio) {
> @@ -2853,6 +2852,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
> sector_t sectors_skipped = 0;
> int chunks_skipped = 0;
> sector_t chunk_mask = conf->geo.chunk_mask;
> + int page_idx = 0;
>
> if (!conf->r10buf_pool)
> if (init_resync(conf))
> @@ -3355,7 +3355,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
> break;
> for (bio= biolist ; bio ; bio=bio->bi_next) {
> struct resync_pages *rp = get_resync_pages(bio);
> - page = resync_fetch_page(rp, rp->idx++);
> + page = resync_fetch_page(rp, page_idx);
> /*
> * won't fail because the vec table is big enough
> * to hold all these pages
> @@ -3364,7 +3364,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
> }
> nr_sectors += len>>9;
> sector_nr += len>>9;
> - } while (get_resync_pages(biolist)->idx < RESYNC_PAGES);
> + } while (page_idx++ < RESYNC_PAGES);
Same problem here.
Otherwise, the patch looks good.
NeilBrown
> r10_bio->sectors = nr_sectors;
>
> while (biolist) {
> --
> 2.9.4
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] md: raid1/raid10: initialize bvec table via bio_add_page()
From: NeilBrown @ 2017-07-13 0:01 UTC (permalink / raw)
To: Shaohua Li, linux-raid
Cc: linux-block, Jens Axboe, Christoph Hellwig, Ming Lei
In-Reply-To: <20170712082912.491-1-ming.lei@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4449 bytes --]
On Wed, Jul 12 2017, Ming Lei wrote:
> We will support multipage bvec soon, so initialize bvec
> table using the standardy way instead of writing the
> talbe directly. Otherwise it won't work any more once
> multipage bvec is enabled.
>
> Acked-by: Guoqing Jiang <gqjiang@suse.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> drivers/md/md.c | 21 +++++++++++++++++++++
> drivers/md/md.h | 3 +++
> drivers/md/raid1.c | 16 ++--------------
> drivers/md/raid10.c | 4 ++--
> 4 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 8cdca0296749..cc8dcd928dde 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr)
> }
> EXPORT_SYMBOL(md_reload_sb);
>
> +/* generally called after bio_reset() for reseting bvec */
> +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
> + int size)
> +{
> + int idx = 0;
> +
> + /* initialize bvec table again */
> + do {
> + struct page *page = resync_fetch_page(rp, idx);
> + int len = min_t(int, size, PAGE_SIZE);
> +
> + /*
> + * won't fail because the vec table is big
> + * enough to hold all these pages
> + */
> + bio_add_page(bio, page, len, 0);
> + size -= len;
> + } while (idx++ < RESYNC_PAGES && size > 0);
> +}
> +EXPORT_SYMBOL(md_bio_reset_resync_pages);
I really don't think this is a good idea.
This code is specific to raid1/raid10. It is not generic md code. So
it doesn't belong here.
If you want to remove code duplication, then work on moving all raid1
functionality into raid10.c, then discard raid1.c
Or at the very least, have a separate "raid1-10.c" file for the common
code.
NeilBrown
> +
> #ifndef MODULE
>
> /*
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 2c780aa8d07f..efb32ce7a2f1 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -782,4 +782,7 @@ static inline struct page *resync_fetch_page(struct resync_pages *rp,
> return NULL;
> return rp->pages[idx];
> }
> +
> +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
> + int size);
> #endif /* _MD_MD_H */
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 7901ddc3362f..5dc3fda2fdf7 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2085,10 +2085,7 @@ static void process_checks(struct r1bio *r1_bio)
> /* Fix variable parts of all bios */
> vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
> for (i = 0; i < conf->raid_disks * 2; i++) {
> - int j;
> - int size;
> blk_status_t status;
> - struct bio_vec *bi;
> struct bio *b = r1_bio->bios[i];
> struct resync_pages *rp = get_resync_pages(b);
> if (b->bi_end_io != end_sync_read)
> @@ -2097,8 +2094,6 @@ static void process_checks(struct r1bio *r1_bio)
> status = b->bi_status;
> bio_reset(b);
> b->bi_status = status;
> - b->bi_vcnt = vcnt;
> - b->bi_iter.bi_size = r1_bio->sectors << 9;
> b->bi_iter.bi_sector = r1_bio->sector +
> conf->mirrors[i].rdev->data_offset;
> b->bi_bdev = conf->mirrors[i].rdev->bdev;
> @@ -2106,15 +2101,8 @@ static void process_checks(struct r1bio *r1_bio)
> rp->raid_bio = r1_bio;
> b->bi_private = rp;
>
> - size = b->bi_iter.bi_size;
> - bio_for_each_segment_all(bi, b, j) {
> - bi->bv_offset = 0;
> - if (size > PAGE_SIZE)
> - bi->bv_len = PAGE_SIZE;
> - else
> - bi->bv_len = size;
> - size -= PAGE_SIZE;
> - }
> + /* initialize bvec table again */
> + md_bio_reset_resync_pages(b, rp, r1_bio->sectors << 9);
> }
> for (primary = 0; primary < conf->raid_disks * 2; primary++)
> if (r1_bio->bios[primary]->bi_end_io == end_sync_read &&
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index e594ca610f27..cb8e803cd1c2 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2086,8 +2086,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
> rp = get_resync_pages(tbio);
> bio_reset(tbio);
>
> - tbio->bi_vcnt = vcnt;
> - tbio->bi_iter.bi_size = fbio->bi_iter.bi_size;
> + md_bio_reset_resync_pages(tbio, rp, fbio->bi_iter.bi_size);
> +
> rp->raid_bio = r10_bio;
> tbio->bi_private = rp;
> tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
> --
> 2.9.4
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH v3 05/14] md: raid1: don't use bio's vec table to manage resync pages
From: Ming Lei @ 2017-07-13 1:22 UTC (permalink / raw)
To: Shaohua Li
Cc: Ming Lei, NeilBrown, Jens Axboe,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig
In-Reply-To: <20170712163050.sxmylv7uq5f2z6gp@kernel.org>
On Wed, Jul 12, 2017 at 09:30:50AM -0700, Shaohua Li wrote:
> On Wed, Jul 12, 2017 at 09:40:10AM +0800, Ming Lei wrote:
> > On Tue, Jul 11, 2017 at 7:14 AM, NeilBrown <neilb@suse.com> wrote:
> > > On Mon, Jul 10 2017, Shaohua Li wrote:
> > >
> > >> On Mon, Jul 10, 2017 at 03:25:41PM +0800, Ming Lei wrote:
> > >>> On Mon, Jul 10, 2017 at 02:38:19PM +1000, NeilBrown wrote:
> > >>> > On Mon, Jul 10 2017, Ming Lei wrote:
> > >>> >
> > >>> > > On Mon, Jul 10, 2017 at 11:35:12AM +0800, Ming Lei wrote:
> > >>> > >> On Mon, Jul 10, 2017 at 7:09 AM, NeilBrown <neilb@suse.com> wrote:
> > >>> > ...
> > >>> > >> >> +
> > >>> > >> >> + rp->idx = 0;
> > >>> > >> >
> > >>> > >> > This is the only place the ->idx is initialized, in r1buf_pool_alloc().
> > >>> > >> > The mempool alloc function is suppose to allocate memory, not initialize
> > >>> > >> > it.
> > >>> > >> >
> > >>> > >> > If the mempool_alloc() call cannot allocate memory it will use memory
> > >>> > >> > from the pool. If this memory has already been used, then it will no
> > >>> > >> > longer have the initialized value.
> > >>> > >> >
> > >>> > >> > In short: you need to initialise memory *after* calling
> > >>> > >> > mempool_alloc(), unless you ensure it is reset to the init values before
> > >>> > >> > calling mempool_free().
> > >>> > >> >
> > >>> > >> > https://bugzilla.kernel.org/show_bug.cgi?id=196307
> > >>> > >>
> > >>> > >> OK, thanks for posting it out.
> > >>> > >>
> > >>> > >> Another fix might be to reinitialize the variable(rp->idx = 0) in
> > >>> > >> r1buf_pool_free().
> > >>> > >> Or just set it as zero every time when it is used.
> > >>> > >>
> > >>> > >> But I don't understand why mempool_free() calls pool->free() at the end of
> > >>> > >> this function, which may cause to run pool->free() on a new allocated buf,
> > >>> > >> seems a bug in mempool?
> > >>> > >
> > >>> > > Looks I missed the 'return' in mempool_free(), so it is fine.
> > >>> > >
> > >>> > > How about the following fix?
> > >>> >
> > >>> > It looks like it would probably work, but it is rather unusual to
> > >>> > initialise something just before freeing it.
> > >>> >
> > >>> > Couldn't you just move the initialization to shortly after the
> > >>> > mempool_alloc() call. There looks like a good place that already loops
> > >>> > over all the bios....
> > >>>
> > >>> OK, follows the revised patch according to your suggestion.
> > >
> > > Thanks.
> > >
> > > That isn't as tidy as I hoped. So I went deeper into the code to try to
> > > understand why...
> > >
> > > I think that maybe we should just discard the ->idx field completely.
> > > It is only used in this code:
> > >
> > > do {
> > > struct page *page;
> > > int len = PAGE_SIZE;
> > > if (sector_nr + (len>>9) > max_sector)
> > > len = (max_sector - sector_nr) << 9;
> > > if (len == 0)
> > > break;
> > > for (bio= biolist ; bio ; bio=bio->bi_next) {
> > > struct resync_pages *rp = get_resync_pages(bio);
> > > page = resync_fetch_page(rp, rp->idx++);
> > > /*
> > > * won't fail because the vec table is big enough
> > > * to hold all these pages
> > > */
> > > bio_add_page(bio, page, len, 0);
> > > }
> > > nr_sectors += len>>9;
> > > sector_nr += len>>9;
> > > } while (get_resync_pages(biolist)->idx < RESYNC_PAGES);
> > >
> > > and all of the different 'rp' always have the same value for 'idx'.
> > > This code is more complex than it needs to be. This is because it used
> > > to be possible for bio_add_page() to fail. That cannot happen any more.
> > > So we can make the code something like:
> > >
> > > for (idx = 0; idx < RESYNC_PAGES; idx++) {
> > > struct page *page;
> > > int len = PAGE_SIZE;
> > > if (sector_nr + (len >> 9) > max_sector)
> > > len = (max_sector - sector_nr) << 9
> > > if (len == 0)
> > > break;
> > > for (bio = biolist; bio; bio = bio->bi_next) {
> > > struct resync_pages *rp = get_resync_pages(bio);
> > > page = resync_fetch_page(rp, idx);
> > > bio_add_page(bio, page, len, 0);
> > > }
> > > nr_sectors += len >> 9;
> > > sector_nr += len >> 9;
> > > }
> > >
> > > Or did I miss something?
> >
> > I think this approach is much clean.
>
> Thought I suggested not using the 'idx' in your previous post, but you said
> there is reason (not because of bio_add_page) not to do it. Is that changed?
> can't remember the details, I need to dig the mail archives.
I found it:
http://marc.info/?l=linux-raid&m=148847751302825&w=2
Not sure why I didn't change to this way in v3, but the idea is correct.
Maybe I misunderstood it that time.
--
Ming
^ permalink raw reply
* Re: [PATCH 2/2] md: raid1/raid10: initialize bvec table via bio_add_page()
From: Ming Lei @ 2017-07-13 1:37 UTC (permalink / raw)
To: NeilBrown
Cc: Shaohua Li, linux-raid, linux-block, Jens Axboe,
Christoph Hellwig
In-Reply-To: <87zic92oiq.fsf@notabene.neil.brown.name>
On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote:
> On Wed, Jul 12 2017, Ming Lei wrote:
>
> > We will support multipage bvec soon, so initialize bvec
> > table using the standardy way instead of writing the
> > talbe directly. Otherwise it won't work any more once
> > multipage bvec is enabled.
> >
> > Acked-by: Guoqing Jiang <gqjiang@suse.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > drivers/md/md.c | 21 +++++++++++++++++++++
> > drivers/md/md.h | 3 +++
> > drivers/md/raid1.c | 16 ++--------------
> > drivers/md/raid10.c | 4 ++--
> > 4 files changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 8cdca0296749..cc8dcd928dde 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr)
> > }
> > EXPORT_SYMBOL(md_reload_sb);
> >
> > +/* generally called after bio_reset() for reseting bvec */
> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
> > + int size)
> > +{
> > + int idx = 0;
> > +
> > + /* initialize bvec table again */
> > + do {
> > + struct page *page = resync_fetch_page(rp, idx);
> > + int len = min_t(int, size, PAGE_SIZE);
> > +
> > + /*
> > + * won't fail because the vec table is big
> > + * enough to hold all these pages
> > + */
> > + bio_add_page(bio, page, len, 0);
> > + size -= len;
> > + } while (idx++ < RESYNC_PAGES && size > 0);
> > +}
> > +EXPORT_SYMBOL(md_bio_reset_resync_pages);
>
> I really don't think this is a good idea.
> This code is specific to raid1/raid10. It is not generic md code. So
> it doesn't belong here.
We already added 'struct resync_pages' in drivers/md/md.h, so I think
it is reasonable to add this function into drivers/md/md.c
>
> If you want to remove code duplication, then work on moving all raid1
> functionality into raid10.c, then discard raid1.c
This patch is for avoiding new code duplication, not for removing current
duplication.
>
> Or at the very least, have a separate "raid1-10.c" file for the common
> code.
You suggested it last time, but looks too overkill to be taken. But if
someone wants to refactor raid1 and raid10, I think it can be a good start,
but still not belong to this patch.
Thanks,
Ming
>
> NeilBrown
>
> > +
> > #ifndef MODULE
> >
> > /*
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index 2c780aa8d07f..efb32ce7a2f1 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -782,4 +782,7 @@ static inline struct page *resync_fetch_page(struct resync_pages *rp,
> > return NULL;
> > return rp->pages[idx];
> > }
> > +
> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
> > + int size);
> > #endif /* _MD_MD_H */
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index 7901ddc3362f..5dc3fda2fdf7 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -2085,10 +2085,7 @@ static void process_checks(struct r1bio *r1_bio)
> > /* Fix variable parts of all bios */
> > vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
> > for (i = 0; i < conf->raid_disks * 2; i++) {
> > - int j;
> > - int size;
> > blk_status_t status;
> > - struct bio_vec *bi;
> > struct bio *b = r1_bio->bios[i];
> > struct resync_pages *rp = get_resync_pages(b);
> > if (b->bi_end_io != end_sync_read)
> > @@ -2097,8 +2094,6 @@ static void process_checks(struct r1bio *r1_bio)
> > status = b->bi_status;
> > bio_reset(b);
> > b->bi_status = status;
> > - b->bi_vcnt = vcnt;
> > - b->bi_iter.bi_size = r1_bio->sectors << 9;
> > b->bi_iter.bi_sector = r1_bio->sector +
> > conf->mirrors[i].rdev->data_offset;
> > b->bi_bdev = conf->mirrors[i].rdev->bdev;
> > @@ -2106,15 +2101,8 @@ static void process_checks(struct r1bio *r1_bio)
> > rp->raid_bio = r1_bio;
> > b->bi_private = rp;
> >
> > - size = b->bi_iter.bi_size;
> > - bio_for_each_segment_all(bi, b, j) {
> > - bi->bv_offset = 0;
> > - if (size > PAGE_SIZE)
> > - bi->bv_len = PAGE_SIZE;
> > - else
> > - bi->bv_len = size;
> > - size -= PAGE_SIZE;
> > - }
> > + /* initialize bvec table again */
> > + md_bio_reset_resync_pages(b, rp, r1_bio->sectors << 9);
> > }
> > for (primary = 0; primary < conf->raid_disks * 2; primary++)
> > if (r1_bio->bios[primary]->bi_end_io == end_sync_read &&
> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> > index e594ca610f27..cb8e803cd1c2 100644
> > --- a/drivers/md/raid10.c
> > +++ b/drivers/md/raid10.c
> > @@ -2086,8 +2086,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
> > rp = get_resync_pages(tbio);
> > bio_reset(tbio);
> >
> > - tbio->bi_vcnt = vcnt;
> > - tbio->bi_iter.bi_size = fbio->bi_iter.bi_size;
> > + md_bio_reset_resync_pages(tbio, rp, fbio->bi_iter.bi_size);
> > +
> > rp->raid_bio = r10_bio;
> > tbio->bi_private = rp;
> > tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
> > --
> > 2.9.4
--
Ming
^ permalink raw reply
* Re: [PATCH 2/2] md: raid1/raid10: initialize bvec table via bio_add_page()
From: Ming Lei @ 2017-07-13 1:40 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Shaohua Li, linux-raid, NeilBrown, linux-block, Jens Axboe
In-Reply-To: <20170712091608.GA27795@infradead.org>
On Wed, Jul 12, 2017 at 02:16:08AM -0700, Christoph Hellwig wrote:
> On Wed, Jul 12, 2017 at 04:29:12PM +0800, Ming Lei wrote:
> > We will support multipage bvec soon, so initialize bvec
> > table using the standardy way instead of writing the
> > talbe directly. Otherwise it won't work any more once
> > multipage bvec is enabled.
>
> It seems to me like these callsites also should use bio_init
> instead of bio_reset to get a clean slate (and eventually
> phase out bio_reset), which should probably got into
> the helper as well. E.g. instead of pretending to preserve
> things we should simply build a new bio here.
At least for resetting bvec table, bio_reset() is enough, so
I think changing to bio_init() should belong to another topic, :-)
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH 1/2] md: remove 'idx' from 'struct resync_pages'
From: Ming Lei @ 2017-07-13 1:41 UTC (permalink / raw)
To: NeilBrown
Cc: Shaohua Li, linux-raid, linux-block, Jens Axboe,
Christoph Hellwig
In-Reply-To: <8737a1437y.fsf@notabene.neil.brown.name>
On Thu, Jul 13, 2017 at 09:58:41AM +1000, NeilBrown wrote:
> On Wed, Jul 12 2017, Ming Lei wrote:
>
> > bio_add_page() won't fail for resync bio, and the page index for each
> > bio is same, so remove it.
> >
> > More importantly the 'idx' of 'struct resync_pages' is initialized in
> > mempool allocator function, this way is wrong since mempool is only
> > responsible for allocation, we can't use that for initialization.
> >
> > Suggested-by: NeilBrown <neilb@suse.com>
> > Reported-by: NeilBrown <neilb@suse.com>
> > Fixes: f0250618361d(md: raid10: don't use bio's vec table to manage resync pages)
> > Fixes: 98d30c5812c3(md: raid1: don't use bio's vec table to manage resync pages)
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > drivers/md/md.h | 1 -
> > drivers/md/raid1.c | 6 +++---
> > drivers/md/raid10.c | 6 +++---
> > 3 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index 991f0fe2dcc6..2c780aa8d07f 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -736,7 +736,6 @@ static inline void mddev_check_write_zeroes(struct mddev *mddev, struct bio *bio
> >
> > /* for managing resync I/O pages */
> > struct resync_pages {
> > - unsigned idx; /* for get/put page from the pool */
> > void *raid_bio;
> > struct page *pages[RESYNC_PAGES];
> > };
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index 3febfc8391fb..7901ddc3362f 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -170,7 +170,6 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
> > resync_get_all_pages(rp);
> > }
> >
> > - rp->idx = 0;
> > rp->raid_bio = r1_bio;
> > bio->bi_private = rp;
> > }
> > @@ -2619,6 +2618,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
> > int good_sectors = RESYNC_SECTORS;
> > int min_bad = 0; /* number of sectors that are bad in all devices */
> > int idx = sector_to_idx(sector_nr);
> > + int page_idx = 0;
> >
> > if (!conf->r1buf_pool)
> > if (init_resync(conf))
> > @@ -2846,7 +2846,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
> > bio = r1_bio->bios[i];
> > rp = get_resync_pages(bio);
> > if (bio->bi_end_io) {
> > - page = resync_fetch_page(rp, rp->idx++);
> > + page = resync_fetch_page(rp, page_idx);
> >
> > /*
> > * won't fail because the vec table is big
> > @@ -2858,7 +2858,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
> > nr_sectors += len>>9;
> > sector_nr += len>>9;
> > sync_blocks -= (len>>9);
> > - } while (get_resync_pages(r1_bio->bios[disk]->bi_private)->idx < RESYNC_PAGES);
> > + } while (page_idx++ < RESYNC_PAGES);
>
> I think you want ++page_idx < RESYNC_PAGES, otherwise there will be
> one pass through the loop where page_idx == RESYNC_PAGES
Good catch, thanks!
thanks,
Ming
^ permalink raw reply
* Re: [PATCH 2/2] md: raid1/raid10: initialize bvec table via bio_add_page()
From: NeilBrown @ 2017-07-13 3:09 UTC (permalink / raw)
To: Ming Lei; +Cc: Shaohua Li, linux-raid, linux-block, Jens Axboe,
Christoph Hellwig
In-Reply-To: <20170713013710.GC670@ming.t460p>
[-- Attachment #1: Type: text/plain, Size: 5861 bytes --]
On Thu, Jul 13 2017, Ming Lei wrote:
> On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote:
>> On Wed, Jul 12 2017, Ming Lei wrote:
>>
>> > We will support multipage bvec soon, so initialize bvec
>> > table using the standardy way instead of writing the
>> > talbe directly. Otherwise it won't work any more once
>> > multipage bvec is enabled.
>> >
>> > Acked-by: Guoqing Jiang <gqjiang@suse.com>
>> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> > ---
>> > drivers/md/md.c | 21 +++++++++++++++++++++
>> > drivers/md/md.h | 3 +++
>> > drivers/md/raid1.c | 16 ++--------------
>> > drivers/md/raid10.c | 4 ++--
>> > 4 files changed, 28 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/drivers/md/md.c b/drivers/md/md.c
>> > index 8cdca0296749..cc8dcd928dde 100644
>> > --- a/drivers/md/md.c
>> > +++ b/drivers/md/md.c
>> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr)
>> > }
>> > EXPORT_SYMBOL(md_reload_sb);
>> >
>> > +/* generally called after bio_reset() for reseting bvec */
>> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
>> > + int size)
>> > +{
>> > + int idx = 0;
>> > +
>> > + /* initialize bvec table again */
>> > + do {
>> > + struct page *page = resync_fetch_page(rp, idx);
>> > + int len = min_t(int, size, PAGE_SIZE);
>> > +
>> > + /*
>> > + * won't fail because the vec table is big
>> > + * enough to hold all these pages
>> > + */
>> > + bio_add_page(bio, page, len, 0);
>> > + size -= len;
>> > + } while (idx++ < RESYNC_PAGES && size > 0);
>> > +}
>> > +EXPORT_SYMBOL(md_bio_reset_resync_pages);
>>
>> I really don't think this is a good idea.
>> This code is specific to raid1/raid10. It is not generic md code. So
>> it doesn't belong here.
>
> We already added 'struct resync_pages' in drivers/md/md.h, so I think
> it is reasonable to add this function into drivers/md/md.c
Alternative perspective: it was unreasonable to add "resync_pages" to
md.h.
>
>>
>> If you want to remove code duplication, then work on moving all raid1
>> functionality into raid10.c, then discard raid1.c
>
> This patch is for avoiding new code duplication, not for removing current
> duplication.
>
>>
>> Or at the very least, have a separate "raid1-10.c" file for the common
>> code.
>
> You suggested it last time, but looks too overkill to be taken. But if
> someone wants to refactor raid1 and raid10, I think it can be a good start,
> but still not belong to this patch.
You are trying to create common code for raid1 and raid10. This does
not belong in md.c.
If you really want to have a single copy of common code, then it exactly
is the role of this patch to create a place to put it.
I'm not saying you should put all common code in raid1-10.c. Just the
function that you have identified.
NeilBrown
>
> Thanks,
> Ming
>
>>
>> NeilBrown
>>
>> > +
>> > #ifndef MODULE
>> >
>> > /*
>> > diff --git a/drivers/md/md.h b/drivers/md/md.h
>> > index 2c780aa8d07f..efb32ce7a2f1 100644
>> > --- a/drivers/md/md.h
>> > +++ b/drivers/md/md.h
>> > @@ -782,4 +782,7 @@ static inline struct page *resync_fetch_page(struct resync_pages *rp,
>> > return NULL;
>> > return rp->pages[idx];
>> > }
>> > +
>> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
>> > + int size);
>> > #endif /* _MD_MD_H */
>> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> > index 7901ddc3362f..5dc3fda2fdf7 100644
>> > --- a/drivers/md/raid1.c
>> > +++ b/drivers/md/raid1.c
>> > @@ -2085,10 +2085,7 @@ static void process_checks(struct r1bio *r1_bio)
>> > /* Fix variable parts of all bios */
>> > vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
>> > for (i = 0; i < conf->raid_disks * 2; i++) {
>> > - int j;
>> > - int size;
>> > blk_status_t status;
>> > - struct bio_vec *bi;
>> > struct bio *b = r1_bio->bios[i];
>> > struct resync_pages *rp = get_resync_pages(b);
>> > if (b->bi_end_io != end_sync_read)
>> > @@ -2097,8 +2094,6 @@ static void process_checks(struct r1bio *r1_bio)
>> > status = b->bi_status;
>> > bio_reset(b);
>> > b->bi_status = status;
>> > - b->bi_vcnt = vcnt;
>> > - b->bi_iter.bi_size = r1_bio->sectors << 9;
>> > b->bi_iter.bi_sector = r1_bio->sector +
>> > conf->mirrors[i].rdev->data_offset;
>> > b->bi_bdev = conf->mirrors[i].rdev->bdev;
>> > @@ -2106,15 +2101,8 @@ static void process_checks(struct r1bio *r1_bio)
>> > rp->raid_bio = r1_bio;
>> > b->bi_private = rp;
>> >
>> > - size = b->bi_iter.bi_size;
>> > - bio_for_each_segment_all(bi, b, j) {
>> > - bi->bv_offset = 0;
>> > - if (size > PAGE_SIZE)
>> > - bi->bv_len = PAGE_SIZE;
>> > - else
>> > - bi->bv_len = size;
>> > - size -= PAGE_SIZE;
>> > - }
>> > + /* initialize bvec table again */
>> > + md_bio_reset_resync_pages(b, rp, r1_bio->sectors << 9);
>> > }
>> > for (primary = 0; primary < conf->raid_disks * 2; primary++)
>> > if (r1_bio->bios[primary]->bi_end_io == end_sync_read &&
>> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> > index e594ca610f27..cb8e803cd1c2 100644
>> > --- a/drivers/md/raid10.c
>> > +++ b/drivers/md/raid10.c
>> > @@ -2086,8 +2086,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
>> > rp = get_resync_pages(tbio);
>> > bio_reset(tbio);
>> >
>> > - tbio->bi_vcnt = vcnt;
>> > - tbio->bi_iter.bi_size = fbio->bi_iter.bi_size;
>> > + md_bio_reset_resync_pages(tbio, rp, fbio->bi_iter.bi_size);
>> > +
>> > rp->raid_bio = r10_bio;
>> > tbio->bi_private = rp;
>> > tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
>> > --
>> > 2.9.4
>
>
>
> --
> Ming
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [mdadm PATCH 1/1] mdadm/test: Add one test case for raid5 reshape
From: Xiao Ni @ 2017-07-13 9:14 UTC (permalink / raw)
To: linux-raid; +Cc: jes.sorensen, shli
This case trys to allow raid5 reshape to use backwards direction.
It changes chunksize after reshape and stop the raid. Then start
the raid again.
Signed-off-by: Xiao Ni <xni@redhat.com>
---
tests/02r5grow | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/tests/02r5grow b/tests/02r5grow
index 386e82e..14d05d6 100644
--- a/tests/02r5grow
+++ b/tests/02r5grow
@@ -34,3 +34,15 @@ check nosync
sh tests/testdev $md0 3 $[size/2] 128
mdadm -S $md0
+
+# create a raid5 array and change the chunk
+mdadm -CR $md0 --level raid5 --metadata=1.1 --chunk=32 --raid-disks 3 --size $[size/2] $dev1 $dev2 $dev3
+check wait
+check state UUU
+
+mdadm $md0 --grow --chunk=64
+mdadm --wait $md0
+
+mdadm -S $md0
+mdadm -As
+mdadm -Ss
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 2/2] md: raid1/raid10: initialize bvec table via bio_add_page()
From: Ming Lei @ 2017-07-13 9:20 UTC (permalink / raw)
To: NeilBrown
Cc: Shaohua Li, linux-raid, linux-block, Jens Axboe,
Christoph Hellwig
In-Reply-To: <87inix2ftj.fsf@notabene.neil.brown.name>
On Thu, Jul 13, 2017 at 01:09:28PM +1000, NeilBrown wrote:
> On Thu, Jul 13 2017, Ming Lei wrote:
>
> > On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote:
> >> On Wed, Jul 12 2017, Ming Lei wrote:
> >>
> >> > We will support multipage bvec soon, so initialize bvec
> >> > table using the standardy way instead of writing the
> >> > talbe directly. Otherwise it won't work any more once
> >> > multipage bvec is enabled.
> >> >
> >> > Acked-by: Guoqing Jiang <gqjiang@suse.com>
> >> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >> > ---
> >> > drivers/md/md.c | 21 +++++++++++++++++++++
> >> > drivers/md/md.h | 3 +++
> >> > drivers/md/raid1.c | 16 ++--------------
> >> > drivers/md/raid10.c | 4 ++--
> >> > 4 files changed, 28 insertions(+), 16 deletions(-)
> >> >
> >> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> > index 8cdca0296749..cc8dcd928dde 100644
> >> > --- a/drivers/md/md.c
> >> > +++ b/drivers/md/md.c
> >> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr)
> >> > }
> >> > EXPORT_SYMBOL(md_reload_sb);
> >> >
> >> > +/* generally called after bio_reset() for reseting bvec */
> >> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
> >> > + int size)
> >> > +{
> >> > + int idx = 0;
> >> > +
> >> > + /* initialize bvec table again */
> >> > + do {
> >> > + struct page *page = resync_fetch_page(rp, idx);
> >> > + int len = min_t(int, size, PAGE_SIZE);
> >> > +
> >> > + /*
> >> > + * won't fail because the vec table is big
> >> > + * enough to hold all these pages
> >> > + */
> >> > + bio_add_page(bio, page, len, 0);
> >> > + size -= len;
> >> > + } while (idx++ < RESYNC_PAGES && size > 0);
> >> > +}
> >> > +EXPORT_SYMBOL(md_bio_reset_resync_pages);
> >>
> >> I really don't think this is a good idea.
> >> This code is specific to raid1/raid10. It is not generic md code. So
> >> it doesn't belong here.
> >
> > We already added 'struct resync_pages' in drivers/md/md.h, so I think
> > it is reasonable to add this function into drivers/md/md.c
>
> Alternative perspective: it was unreasonable to add "resync_pages" to
> md.h.
>
> >
> >>
> >> If you want to remove code duplication, then work on moving all raid1
> >> functionality into raid10.c, then discard raid1.c
> >
> > This patch is for avoiding new code duplication, not for removing current
> > duplication.
> >
> >>
> >> Or at the very least, have a separate "raid1-10.c" file for the common
> >> code.
> >
> > You suggested it last time, but looks too overkill to be taken. But if
> > someone wants to refactor raid1 and raid10, I think it can be a good start,
> > but still not belong to this patch.
>
> You are trying to create common code for raid1 and raid10. This does
> not belong in md.c.
> If you really want to have a single copy of common code, then it exactly
> is the role of this patch to create a place to put it.
> I'm not saying you should put all common code in raid1-10.c. Just the
> function that you have identified.
I really don't want to waste time on this kind of thing, I can do
either one frankly.
Shaohua, could you share me which way you like to merge? I can do it in
either way.
--
Ming
^ permalink raw reply
* Re: [mdadm PATCH 1/1] mdadm/test: Add one test case for raid5 reshape
From: Jes Sorensen @ 2017-07-13 14:27 UTC (permalink / raw)
To: Xiao Ni, linux-raid; +Cc: shli
In-Reply-To: <1499937259-16791-1-git-send-email-xni@redhat.com>
On 07/13/2017 05:14 AM, Xiao Ni wrote:
> This case trys to allow raid5 reshape to use backwards direction.
> It changes chunksize after reshape and stop the raid. Then start
> the raid again.
>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
> tests/02r5grow | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/tests/02r5grow b/tests/02r5grow
> index 386e82e..14d05d6 100644
> --- a/tests/02r5grow
> +++ b/tests/02r5grow
> @@ -34,3 +34,15 @@ check nosync
> sh tests/testdev $md0 3 $[size/2] 128
>
> mdadm -S $md0
> +
> +# create a raid5 array and change the chunk
> +mdadm -CR $md0 --level raid5 --metadata=1.1 --chunk=32 --raid-disks 3 --size $[size/2] $dev1 $dev2 $dev3
> +check wait
> +check state UUU
> +
> +mdadm $md0 --grow --chunk=64
> +mdadm --wait $md0
You are not checking the outcome of your chunk change operation. Running
a test and not making sure it succeeds kinda makes it useless.
Cheers,
Jes
> +
> +mdadm -S $md0
> +mdadm -As
> +mdadm -Ss
>
^ permalink raw reply
* Filesystem corruption on RAID1
From: Gionatan Danti @ 2017-07-13 15:35 UTC (permalink / raw)
To: linux-raid; +Cc: g.danti
Hi list,
today I had an unexpected filesystem corruption on a RAID1 machine used
for backup purposes. I would like to reconstruct what possibly happened
on why, so I am asking for your help.
System specs:
- OS CentOS 7.2 x86_64 with kernel 3.10.0-514.6.1.el7.x86_64
- 2x SEAGATE ST4000VN000-1H4168 (4 TB 5900rpm disks)
- 4 GB DDR3 RAM
- Intel(R) Pentium(R) CPU G3260 @ 3.30GHz
Today, I found the machine crashed with an XFS warning about corrupted
metadata. The warning stated that in-core (or in-memory) data corruption
was detected so, thinking about a DRAM-related problem (no ECC memory on
this small box...) I simply rebooted tha machine. To no avail - the same
problem immediately happened, preventing the machine from booting (the
root filesystem did not mount).
After the filesystem was repaired (with significant corruption signs,
also due to the clearing of the XFS journal), I looked at dmesg and
found something interesting: a raid-resync action was *automatically*
performed, as when re-attaching a (detached) disk.
I start investigating in /var/log/messages and found plenty of these
errors, spanning many days:
...
Jul 10 03:24:01 nas kernel: ata1.00: failed command: READ FPDMA QUEUED
Jul 10 14:50:54 nas kernel: ata1.00: failed command: FLUSH CACHE EXT
Jul 12 03:14:41 nas kernel: ata1.00: failed command: WRITE FPDMA QUEUED
...
To me, it seems that a disks (the first one, sda) had problem executing
some SATA commands, becoming out-of-sync from the second one (sdb).
However it was not kicked out the array, as both /var/log/messages *and*
my custom monitoring script (which keep an eye on /proc/mdstat) reported
nothing. Moreover, inspecting both the SMART values and log show *no*
error at all.
Question 1: it is possible to have such a situation, where a failed
command *silently* put the array in out-of-sync state?
At a certain point, the machine crashed. I noticed and rebooted it.
Question 2: it is possible that the old disk become offline just before
the crash and, by rebooting, the mdadm re-added it to the array?
Question 3: if so, it is possible that the corruption was due to the
first disk being the one read by the md array and, by extension, by the
filesystem?
Any thoughts will be greatly appreciated.
Thanks.
--
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8
^ permalink raw reply
* Re: [PATCH 2/2] md: raid1/raid10: initialize bvec table via bio_add_page()
From: Shaohua Li @ 2017-07-13 16:39 UTC (permalink / raw)
To: Ming Lei
Cc: NeilBrown, linux-raid, linux-block, Jens Axboe, Christoph Hellwig
In-Reply-To: <20170713092050.GG670@ming.t460p>
On Thu, Jul 13, 2017 at 05:20:52PM +0800, Ming Lei wrote:
> On Thu, Jul 13, 2017 at 01:09:28PM +1000, NeilBrown wrote:
> > On Thu, Jul 13 2017, Ming Lei wrote:
> >
> > > On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote:
> > >> On Wed, Jul 12 2017, Ming Lei wrote:
> > >>
> > >> > We will support multipage bvec soon, so initialize bvec
> > >> > table using the standardy way instead of writing the
> > >> > talbe directly. Otherwise it won't work any more once
> > >> > multipage bvec is enabled.
> > >> >
> > >> > Acked-by: Guoqing Jiang <gqjiang@suse.com>
> > >> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > >> > ---
> > >> > drivers/md/md.c | 21 +++++++++++++++++++++
> > >> > drivers/md/md.h | 3 +++
> > >> > drivers/md/raid1.c | 16 ++--------------
> > >> > drivers/md/raid10.c | 4 ++--
> > >> > 4 files changed, 28 insertions(+), 16 deletions(-)
> > >> >
> > >> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > >> > index 8cdca0296749..cc8dcd928dde 100644
> > >> > --- a/drivers/md/md.c
> > >> > +++ b/drivers/md/md.c
> > >> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr)
> > >> > }
> > >> > EXPORT_SYMBOL(md_reload_sb);
> > >> >
> > >> > +/* generally called after bio_reset() for reseting bvec */
> > >> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
> > >> > + int size)
> > >> > +{
> > >> > + int idx = 0;
> > >> > +
> > >> > + /* initialize bvec table again */
> > >> > + do {
> > >> > + struct page *page = resync_fetch_page(rp, idx);
> > >> > + int len = min_t(int, size, PAGE_SIZE);
> > >> > +
> > >> > + /*
> > >> > + * won't fail because the vec table is big
> > >> > + * enough to hold all these pages
> > >> > + */
> > >> > + bio_add_page(bio, page, len, 0);
> > >> > + size -= len;
> > >> > + } while (idx++ < RESYNC_PAGES && size > 0);
> > >> > +}
> > >> > +EXPORT_SYMBOL(md_bio_reset_resync_pages);
> > >>
> > >> I really don't think this is a good idea.
> > >> This code is specific to raid1/raid10. It is not generic md code. So
> > >> it doesn't belong here.
> > >
> > > We already added 'struct resync_pages' in drivers/md/md.h, so I think
> > > it is reasonable to add this function into drivers/md/md.c
> >
> > Alternative perspective: it was unreasonable to add "resync_pages" to
> > md.h.
> >
> > >
> > >>
> > >> If you want to remove code duplication, then work on moving all raid1
> > >> functionality into raid10.c, then discard raid1.c
> > >
> > > This patch is for avoiding new code duplication, not for removing current
> > > duplication.
> > >
> > >>
> > >> Or at the very least, have a separate "raid1-10.c" file for the common
> > >> code.
> > >
> > > You suggested it last time, but looks too overkill to be taken. But if
> > > someone wants to refactor raid1 and raid10, I think it can be a good start,
> > > but still not belong to this patch.
> >
> > You are trying to create common code for raid1 and raid10. This does
> > not belong in md.c.
> > If you really want to have a single copy of common code, then it exactly
> > is the role of this patch to create a place to put it.
> > I'm not saying you should put all common code in raid1-10.c. Just the
> > function that you have identified.
>
> I really don't want to waste time on this kind of thing, I can do
> either one frankly.
>
> Shaohua, could you share me which way you like to merge? I can do it in
> either way.
I don't have strong preference, but Neil's suggestion does make the code a
little better. Of course, only put the function into the raid1-10.c right now.
Thanks,
Shaohua
^ permalink raw reply
* Re: Filesystem corruption on RAID1
From: Roman Mamedov @ 2017-07-13 16:48 UTC (permalink / raw)
To: Gionatan Danti; +Cc: linux-raid
In-Reply-To: <c2fe6593-c806-ab9f-fcff-8327c013237b@assyoma.it>
On Thu, 13 Jul 2017 17:35:12 +0200
Gionatan Danti <g.danti@assyoma.it> wrote:
> Jul 10 03:24:01 nas kernel: ata1.00: failed command: READ FPDMA QUEUED
Failed reads are not as bad, as they are just retried.
> Jul 12 03:14:41 nas kernel: ata1.00: failed command: WRITE FPDMA QUEUED
But these WILL cause incorrect data written to disk, in my experience. After
that, one of your disks will contain some corruption, whether in files, or (as
you discovered) in the filesystem itself. mdadm may or may not read from that
disk, as it chooses the mirror for reads pretty much randomly, using the least
loaded one. And even though the other disk still contains good data, there is
no mechanism for the user-space to say "hey, this doesn't look right, what's
on the other mirror?"
Check your cables and/or disks themselves.
If you know that only one disk had these write errors all the time, you could
try disconnecting it from mirror, and checking if you can get a more
consistent view of the filesystem on the remaining one.
P.S: about my case (which I witnessed on a RAID6):
* copy a file to the array, one disk will hit tons of WRITE FPDMA QUEUED
errors (due to insufficient power and/or bad data cable).
* the file that was just copied, turns out to be corrupted when reading back.
* the problem disk WILL NOT get kicked from the array during this.
--
With respect,
Roman
^ permalink raw reply
* [PATCH 0/2] md/raid6: improvements for ARM/arm64
From: Ard Biesheuvel @ 2017-07-13 17:15 UTC (permalink / raw)
To: linux-arm-kernel, linux-raid
Cc: shli, stockhausen, linux, will.deacon, catalin.marinas,
Ard Biesheuvel
1. Use a faster algorithm for the delta syndrome
2. Implement recovery routines in NEON
As before, NEON intrinsics are used, which means the same code can be
compiled for ARM as well as arm64.
Given that there does not seem to be a maintainer for lib/raid6, could
we take this through one of the ARM trees instead?
Ard Biesheuvel (2):
md/raid6: use faster multiplication for ARM NEON delta syndrome
md/raid6: implement recovery using ARM NEON intrinsics
include/linux/raid/pq.h | 1 +
lib/raid6/Makefile | 4 +-
lib/raid6/algos.c | 3 +
lib/raid6/neon.uc | 33 +++++-
lib/raid6/recov_neon.c | 110 ++++++++++++++++++
lib/raid6/recov_neon_inner.c | 117 ++++++++++++++++++++
6 files changed, 264 insertions(+), 4 deletions(-)
create mode 100644 lib/raid6/recov_neon.c
create mode 100644 lib/raid6/recov_neon_inner.c
--
2.9.3
^ permalink raw reply
* [PATCH 1/2] md/raid6: use faster multiplication for ARM NEON delta syndrome
From: Ard Biesheuvel @ 2017-07-13 17:16 UTC (permalink / raw)
To: linux-arm-kernel, linux-raid
Cc: shli, stockhausen, linux, will.deacon, catalin.marinas,
Ard Biesheuvel
In-Reply-To: <20170713171601.25440-1-ard.biesheuvel@linaro.org>
The P/Q left side optimization in the delta syndrome simply involves
repeatedly multiplying a value by polynomial 'x' in GF(2^8). Given
that 'x * x * x * x' equals 'x^4' even in the polynomial world, we
can accelerate this substantially by performing up to 4 such operations
at once, using the NEON instructions for polynomial multiplication.
Results on a Cortex-A57 running in 64-bit mode:
Before:
-------
raid6: neonx1 xor() 1680 MB/s
raid6: neonx2 xor() 2286 MB/s
raid6: neonx4 xor() 3162 MB/s
raid6: neonx8 xor() 3389 MB/s
After:
------
raid6: neonx1 xor() 2281 MB/s
raid6: neonx2 xor() 3362 MB/s
raid6: neonx4 xor() 3787 MB/s
raid6: neonx8 xor() 4239 MB/s
While we're at it, simplify MASK() by using a signed shift rather than
a vector compare involving a temp register.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
lib/raid6/neon.uc | 33 ++++++++++++++++++--
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/lib/raid6/neon.uc b/lib/raid6/neon.uc
index 4fa51b761dd0..d5242f544551 100644
--- a/lib/raid6/neon.uc
+++ b/lib/raid6/neon.uc
@@ -46,8 +46,12 @@ static inline unative_t SHLBYTE(unative_t v)
*/
static inline unative_t MASK(unative_t v)
{
- const uint8x16_t temp = NBYTES(0);
- return (unative_t)vcltq_s8((int8x16_t)v, (int8x16_t)temp);
+ return (unative_t)vshrq_n_s8((int8x16_t)v, 7);
+}
+
+static inline unative_t PMUL(unative_t v, unative_t u)
+{
+ return (unative_t)vmulq_p8((poly8x16_t)v, (poly8x16_t)u);
}
void raid6_neon$#_gen_syndrome_real(int disks, unsigned long bytes, void **ptrs)
@@ -110,7 +114,30 @@ void raid6_neon$#_xor_syndrome_real(int disks, int start, int stop,
wq$$ = veorq_u8(w1$$, wd$$);
}
/* P/Q left side optimization */
- for ( z = start-1 ; z >= 0 ; z-- ) {
+ for ( z = start-1 ; z >= 3 ; z -= 4 ) {
+ w2$$ = vshrq_n_u8(wq$$, 4);
+ w1$$ = vshlq_n_u8(wq$$, 4);
+
+ w2$$ = PMUL(w2$$, x1d);
+ wq$$ = veorq_u8(w1$$, w2$$);
+ }
+
+ switch (z) {
+ case 2:
+ w2$$ = vshrq_n_u8(wq$$, 5);
+ w1$$ = vshlq_n_u8(wq$$, 3);
+
+ w2$$ = PMUL(w2$$, x1d);
+ wq$$ = veorq_u8(w1$$, w2$$);
+ break;
+ case 1:
+ w2$$ = vshrq_n_u8(wq$$, 6);
+ w1$$ = vshlq_n_u8(wq$$, 2);
+
+ w2$$ = PMUL(w2$$, x1d);
+ wq$$ = veorq_u8(w1$$, w2$$);
+ break;
+ case 0:
w2$$ = MASK(wq$$);
w1$$ = SHLBYTE(wq$$);
--
2.9.3
^ permalink raw reply related
* [PATCH 2/2] md/raid6: implement recovery using ARM NEON intrinsics
From: Ard Biesheuvel @ 2017-07-13 17:16 UTC (permalink / raw)
To: linux-arm-kernel, linux-raid
Cc: shli, stockhausen, linux, will.deacon, catalin.marinas,
Ard Biesheuvel
In-Reply-To: <20170713171601.25440-1-ard.biesheuvel@linaro.org>
Provide a NEON accelerated implementation of the recovery algorithm,
which supersedes the default byte-by-byte one.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
include/linux/raid/pq.h | 1 +
lib/raid6/Makefile | 4 +-
lib/raid6/algos.c | 3 +
lib/raid6/recov_neon.c | 110 ++++++++++++++++++
lib/raid6/recov_neon_inner.c | 117 ++++++++++++++++++++
5 files changed, 234 insertions(+), 1 deletion(-)
diff --git a/include/linux/raid/pq.h b/include/linux/raid/pq.h
index 4d57bbaaa1bf..64318fa49126 100644
--- a/include/linux/raid/pq.h
+++ b/include/linux/raid/pq.h
@@ -121,6 +121,7 @@ extern const struct raid6_recov_calls raid6_recov_ssse3;
extern const struct raid6_recov_calls raid6_recov_avx2;
extern const struct raid6_recov_calls raid6_recov_avx512;
extern const struct raid6_recov_calls raid6_recov_s390xc;
+extern const struct raid6_recov_calls raid6_recov_neon;
extern const struct raid6_calls raid6_neonx1;
extern const struct raid6_calls raid6_neonx2;
diff --git a/lib/raid6/Makefile b/lib/raid6/Makefile
index 3057011f5599..a93adf6dcfb2 100644
--- a/lib/raid6/Makefile
+++ b/lib/raid6/Makefile
@@ -5,7 +5,7 @@ raid6_pq-y += algos.o recov.o tables.o int1.o int2.o int4.o \
raid6_pq-$(CONFIG_X86) += recov_ssse3.o recov_avx2.o mmx.o sse1.o sse2.o avx2.o avx512.o recov_avx512.o
raid6_pq-$(CONFIG_ALTIVEC) += altivec1.o altivec2.o altivec4.o altivec8.o
-raid6_pq-$(CONFIG_KERNEL_MODE_NEON) += neon.o neon1.o neon2.o neon4.o neon8.o
+raid6_pq-$(CONFIG_KERNEL_MODE_NEON) += neon.o neon1.o neon2.o neon4.o neon8.o recov_neon.o recov_neon_inner.o
raid6_pq-$(CONFIG_TILEGX) += tilegx8.o
raid6_pq-$(CONFIG_S390) += s390vx8.o recov_s390xc.o
@@ -26,7 +26,9 @@ NEON_FLAGS := -ffreestanding
ifeq ($(ARCH),arm)
NEON_FLAGS += -mfloat-abi=softfp -mfpu=neon
endif
+CFLAGS_recov_neon_inner.o += $(NEON_FLAGS)
ifeq ($(ARCH),arm64)
+CFLAGS_REMOVE_recov_neon_inner.o += -mgeneral-regs-only
CFLAGS_REMOVE_neon1.o += -mgeneral-regs-only
CFLAGS_REMOVE_neon2.o += -mgeneral-regs-only
CFLAGS_REMOVE_neon4.o += -mgeneral-regs-only
diff --git a/lib/raid6/algos.c b/lib/raid6/algos.c
index 7857049fd7d3..476994723258 100644
--- a/lib/raid6/algos.c
+++ b/lib/raid6/algos.c
@@ -113,6 +113,9 @@ const struct raid6_recov_calls *const raid6_recov_algos[] = {
#ifdef CONFIG_S390
&raid6_recov_s390xc,
#endif
+#if defined(CONFIG_KERNEL_MODE_NEON)
+ &raid6_recov_neon,
+#endif
&raid6_recov_intx1,
NULL
};
diff --git a/lib/raid6/recov_neon.c b/lib/raid6/recov_neon.c
new file mode 100644
index 000000000000..eeb5c4065b92
--- /dev/null
+++ b/lib/raid6/recov_neon.c
@@ -0,0 +1,110 @@
+/*
+ * Copyright (C) 2012 Intel Corporation
+ * Copyright (C) 2017 Linaro Ltd. <ard.biesheuvel@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#include <linux/raid/pq.h>
+
+#ifdef __KERNEL__
+#include <asm/neon.h>
+#else
+#define kernel_neon_begin()
+#define kernel_neon_end()
+#define cpu_has_neon() (1)
+#endif
+
+static int raid6_has_neon(void)
+{
+ return cpu_has_neon();
+}
+
+void __raid6_2data_recov_neon(int bytes, uint8_t *p, uint8_t *q, uint8_t *dp,
+ uint8_t *dq, const uint8_t *pbmul,
+ const uint8_t *qmul);
+
+void __raid6_datap_recov_neon(int bytes, uint8_t *p, uint8_t *q, uint8_t *dq,
+ const uint8_t *qmul);
+
+static void raid6_2data_recov_neon(int disks, size_t bytes, int faila,
+ int failb, void **ptrs)
+{
+ u8 *p, *q, *dp, *dq;
+ const u8 *pbmul; /* P multiplier table for B data */
+ const u8 *qmul; /* Q multiplier table (for both) */
+
+ p = (u8 *)ptrs[disks - 2];
+ q = (u8 *)ptrs[disks - 1];
+
+ /*
+ * Compute syndrome with zero for the missing data pages
+ * Use the dead data pages as temporary storage for
+ * delta p and delta q
+ */
+ dp = (u8 *)ptrs[faila];
+ ptrs[faila] = (void *)raid6_empty_zero_page;
+ ptrs[disks - 2] = dp;
+ dq = (u8 *)ptrs[failb];
+ ptrs[failb] = (void *)raid6_empty_zero_page;
+ ptrs[disks - 1] = dq;
+
+ raid6_call.gen_syndrome(disks, bytes, ptrs);
+
+ /* Restore pointer table */
+ ptrs[faila] = dp;
+ ptrs[failb] = dq;
+ ptrs[disks - 2] = p;
+ ptrs[disks - 1] = q;
+
+ /* Now, pick the proper data tables */
+ pbmul = raid6_vgfmul[raid6_gfexi[failb-faila]];
+ qmul = raid6_vgfmul[raid6_gfinv[raid6_gfexp[faila] ^
+ raid6_gfexp[failb]]];
+
+ kernel_neon_begin();
+ __raid6_2data_recov_neon(bytes, p, q, dp, dq, pbmul, qmul);
+ kernel_neon_end();
+}
+
+static void raid6_datap_recov_neon(int disks, size_t bytes, int faila,
+ void **ptrs)
+{
+ u8 *p, *q, *dq;
+ const u8 *qmul; /* Q multiplier table */
+
+ p = (u8 *)ptrs[disks - 2];
+ q = (u8 *)ptrs[disks - 1];
+
+ /*
+ * Compute syndrome with zero for the missing data page
+ * Use the dead data page as temporary storage for delta q
+ */
+ dq = (u8 *)ptrs[faila];
+ ptrs[faila] = (void *)raid6_empty_zero_page;
+ ptrs[disks - 1] = dq;
+
+ raid6_call.gen_syndrome(disks, bytes, ptrs);
+
+ /* Restore pointer table */
+ ptrs[faila] = dq;
+ ptrs[disks - 1] = q;
+
+ /* Now, pick the proper data tables */
+ qmul = raid6_vgfmul[raid6_gfinv[raid6_gfexp[faila]]];
+
+ kernel_neon_begin();
+ __raid6_datap_recov_neon(bytes, p, q, dq, qmul);
+ kernel_neon_end();
+}
+
+const struct raid6_recov_calls raid6_recov_neon = {
+ .data2 = raid6_2data_recov_neon,
+ .datap = raid6_datap_recov_neon,
+ .valid = raid6_has_neon,
+ .name = "neon",
+ .priority = 10,
+};
diff --git a/lib/raid6/recov_neon_inner.c b/lib/raid6/recov_neon_inner.c
new file mode 100644
index 000000000000..8cd20c9f834a
--- /dev/null
+++ b/lib/raid6/recov_neon_inner.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2012 Intel Corporation
+ * Copyright (C) 2017 Linaro Ltd. <ard.biesheuvel@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#include <arm_neon.h>
+
+static const uint8x16_t x0f = {
+ 0x0f, 0x0f, 0x0f, 0x0f, 0x0f, 0x0f, 0x0f, 0x0f,
+ 0x0f, 0x0f, 0x0f, 0x0f, 0x0f, 0x0f, 0x0f, 0x0f,
+};
+
+#ifdef CONFIG_ARM
+/*
+ * AArch32 does not provide this intrinsic natively because it does not
+ * implement the underlying instruction. AArch32 only provides a 64-bit
+ * wide vtbl.8 instruction, so use that instead.
+ */
+static uint8x16_t vqtbl1q_u8(uint8x16_t a, uint8x16_t b)
+{
+ union {
+ uint8x16_t val;
+ uint8x8x2_t pair;
+ } __a = { a };
+
+ return vcombine_u8(vtbl2_u8(__a.pair, vget_low_u8(b)),
+ vtbl2_u8(__a.pair, vget_high_u8(b)));
+}
+#endif
+
+void __raid6_2data_recov_neon(int bytes, uint8_t *p, uint8_t *q, uint8_t *dp,
+ uint8_t *dq, const uint8_t *pbmul,
+ const uint8_t *qmul)
+{
+ uint8x16_t pm0 = vld1q_u8(pbmul);
+ uint8x16_t pm1 = vld1q_u8(pbmul + 16);
+ uint8x16_t qm0 = vld1q_u8(qmul);
+ uint8x16_t qm1 = vld1q_u8(qmul + 16);
+
+ /*
+ * while ( bytes-- ) {
+ * uint8_t px, qx, db;
+ *
+ * px = *p ^ *dp;
+ * qx = qmul[*q ^ *dq];
+ * *dq++ = db = pbmul[px] ^ qx;
+ * *dp++ = db ^ px;
+ * p++; q++;
+ * }
+ */
+
+ while (bytes) {
+ uint8x16_t vx, vy, px, qx, db;
+
+ px = veorq_u8(vld1q_u8(p), vld1q_u8(dp));
+ vx = veorq_u8(vld1q_u8(q), vld1q_u8(dq));
+
+ vy = (uint8x16_t)vshrq_n_s16((int16x8_t)vx, 4);
+ vx = vqtbl1q_u8(qm0, vandq_u8(vx, x0f));
+ vy = vqtbl1q_u8(qm1, vandq_u8(vy, x0f));
+ qx = veorq_u8(vx, vy);
+
+ vy = (uint8x16_t)vshrq_n_s16((int16x8_t)px, 4);
+ vx = vqtbl1q_u8(pm0, vandq_u8(px, x0f));
+ vy = vqtbl1q_u8(pm1, vandq_u8(vy, x0f));
+ vx = veorq_u8(vx, vy);
+ db = veorq_u8(vx, qx);
+
+ vst1q_u8(dq, db);
+ vst1q_u8(dp, veorq_u8(db, px));
+
+ bytes -= 16;
+ p += 16;
+ q += 16;
+ dp += 16;
+ dq += 16;
+ }
+}
+
+void __raid6_datap_recov_neon(int bytes, uint8_t *p, uint8_t *q, uint8_t *dq,
+ const uint8_t *qmul)
+{
+ uint8x16_t qm0 = vld1q_u8(qmul);
+ uint8x16_t qm1 = vld1q_u8(qmul + 16);
+
+ /*
+ * while (bytes--) {
+ * *p++ ^= *dq = qmul[*q ^ *dq];
+ * q++; dq++;
+ * }
+ */
+
+ while (bytes) {
+ uint8x16_t vx, vy;
+
+ vx = veorq_u8(vld1q_u8(q), vld1q_u8(dq));
+
+ vy = (uint8x16_t)vshrq_n_s16((int16x8_t)vx, 4);
+ vx = vqtbl1q_u8(qm0, vandq_u8(vx, x0f));
+ vy = vqtbl1q_u8(qm1, vandq_u8(vy, x0f));
+ vx = veorq_u8(vx, vy);
+ vy = veorq_u8(vx, vld1q_u8(p));
+
+ vst1q_u8(dq, vx);
+ vst1q_u8(p, vy);
+
+ bytes -= 16;
+ p += 16;
+ q += 16;
+ dq += 16;
+ }
+}
--
2.9.3
^ permalink raw reply related
* AW: [PATCH 1/2] md/raid6: use faster multiplication for ARM NEON delta syndrome
From: Markus Stockhausen @ 2017-07-13 17:51 UTC (permalink / raw)
To: Ard Biesheuvel, linux-arm-kernel@lists.infradead.org,
linux-raid@vger.kernel.org
Cc: will.deacon@arm.com, shli@kernel.org, linux@armlinux.org.uk,
catalin.marinas@arm.com
In-Reply-To: <20170713171601.25440-2-ard.biesheuvel@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]
> Von: Ard Biesheuvel [ard.biesheuvel@linaro.org]
> Gesendet: Donnerstag, 13. Juli 2017 19:16
> An: linux-arm-kernel@lists.infradead.org; linux-raid@vger.kernel.org
> Cc: shli@kernel.org; Markus Stockhausen; linux@armlinux.org.uk; will.deacon@arm.com; catalin.marinas@arm.com; Ard Biesheuvel
> Betreff: [PATCH 1/2] md/raid6: use faster multiplication for ARM NEON delta syndrome
>
> The P/Q left side optimization in the delta syndrome simply involves
> repeatedly multiplying a value by polynomial 'x' in GF(2^8). Given
> that 'x * x * x * x' equals 'x^4' even in the polynomial world, we
> can accelerate this substantially by performing up to 4 such operations
> at once, using the NEON instructions for polynomial multiplication.
>
> Results on a Cortex-A57 running in 64-bit mode:
>
> Before:
> -------
> raid6: neonx1 xor() 1680 MB/s
> raid6: neonx2 xor() 2286 MB/s
> raid6: neonx4 xor() 3162 MB/s
> raid6: neonx8 xor() 3389 MB/s
>
> After:
> ------
> raid6: neonx1 xor() 2281 MB/s
> raid6: neonx2 xor() 3362 MB/s
> raid6: neonx4 xor() 3787 MB/s
> raid6: neonx8 xor() 4239 MB/s
Nice optimiziation. Nevertheless the test algorithm favours this implementation. See:
int start = (disks>>1)-1, stop = disks-3; /* work on the second half of the disks */
What gives the before/after test if you work on the middle data disks and not on
the right ones? In the 4K page size this should be start = 3, stop = 11 instead of
start = 7, stop = 13. Given the large gain you see the impact should be lower but
at least in the >10% range.
Markus
> While we're at it, simplify MASK() by using a signed shift rather than
> a vector compare involving a temp register.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ...=
[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 1650 bytes --]
****************************************************************************
Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte
Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail
irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte
Weitergabe dieser Mail ist nicht gestattet.
Über das Internet versandte E-Mails können unter fremden Namen erstellt oder
manipuliert werden. Deshalb ist diese als E-Mail verschickte Nachricht keine
rechtsverbindliche Willenserklärung.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
Vorstand:
Kadir Akin
Dr. Michael Höhnerbach
Vorsitzender des Aufsichtsrates:
Hans Kristian Langva
Registergericht: Amtsgericht Köln
Registernummer: HRB 52 497
This e-mail may contain confidential and/or privileged information. If you
are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail. Any
unauthorized copying, disclosure or distribution of the material in this
e-mail is strictly forbidden.
e-mails sent over the internet may have been written under a wrong name or
been manipulated. That is why this message sent as an e-mail is not a
legally binding declaration of intention.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
executive board:
Kadir Akin
Dr. Michael Höhnerbach
President of the supervisory board:
Hans Kristian Langva
Registry office: district court Cologne
Register number: HRB 52 497
****************************************************************************
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/2] md/raid6: use faster multiplication for ARM NEON delta syndrome
From: Ard Biesheuvel @ 2017-07-13 18:00 UTC (permalink / raw)
To: Markus Stockhausen
Cc: linux-arm-kernel@lists.infradead.org, linux-raid@vger.kernel.org,
shli@kernel.org, linux@armlinux.org.uk, will.deacon@arm.com,
catalin.marinas@arm.com
In-Reply-To: <12EF8D94C6F8734FB2FF37B9FBEDD173010E029C61@EXCHANGE.collogia.de>
On 13 July 2017 at 18:51, Markus Stockhausen <stockhausen@collogia.de> wrote:
>> Von: Ard Biesheuvel [ard.biesheuvel@linaro.org]
>> Gesendet: Donnerstag, 13. Juli 2017 19:16
>> An: linux-arm-kernel@lists.infradead.org; linux-raid@vger.kernel.org
>> Cc: shli@kernel.org; Markus Stockhausen; linux@armlinux.org.uk; will.deacon@arm.com; catalin.marinas@arm.com; Ard Biesheuvel
>> Betreff: [PATCH 1/2] md/raid6: use faster multiplication for ARM NEON delta syndrome
>>
>> The P/Q left side optimization in the delta syndrome simply involves
>> repeatedly multiplying a value by polynomial 'x' in GF(2^8). Given
>> that 'x * x * x * x' equals 'x^4' even in the polynomial world, we
>> can accelerate this substantially by performing up to 4 such operations
>> at once, using the NEON instructions for polynomial multiplication.
>>
>> Results on a Cortex-A57 running in 64-bit mode:
>>
>> Before:
>> -------
>> raid6: neonx1 xor() 1680 MB/s
>> raid6: neonx2 xor() 2286 MB/s
>> raid6: neonx4 xor() 3162 MB/s
>> raid6: neonx8 xor() 3389 MB/s
>>
>> After:
>> ------
>> raid6: neonx1 xor() 2281 MB/s
>> raid6: neonx2 xor() 3362 MB/s
>> raid6: neonx4 xor() 3787 MB/s
>> raid6: neonx8 xor() 4239 MB/s
>
> Nice optimiziation. Nevertheless the test algorithm favours this implementation. See:
>
> int start = (disks>>1)-1, stop = disks-3; /* work on the second half of the disks */
>
> What gives the before/after test if you work on the middle data disks and not on
> the right ones? In the 4K page size this should be start = 3, stop = 11 instead of
> start = 7, stop = 13. Given the large gain you see the impact should be lower but
> at least in the >10% range.
>
Relative before and after (using raid6test rather than the kernel
module this time, so they should not be compared with the numbers
above)
before
raid6: neonx1 xor() 1773 MB/s
raid6: neonx2 xor() 2362 MB/s
raid6: neonx4 xor() 3223 MB/s
raid6: neonx8 xor() 3375 MB/s
after
raid6: neonx1 xor() 2259 MB/s
raid6: neonx2 xor() 2975 MB/s
raid6: neonx4 xor() 3404 MB/s
raid6: neonx8 xor() 3788 MB/s
So your estimate is correct: 12% speedup for neonx8 in the 'start = 7,
stop = 13' case
--
Ard.
^ permalink raw reply
* Re: Filesystem corruption on RAID1
From: Gionatan Danti @ 2017-07-13 21:28 UTC (permalink / raw)
To: Roman Mamedov; +Cc: linux-raid, g.danti
In-Reply-To: <20170713214856.4a5c8778@natsu>
Il 13-07-2017 18:48 Roman Mamedov ha scritto:
>
> Failed reads are not as bad, as they are just retried.
>
I agree, I reported them only to give a broad picture of the system
state :)
>> Jul 12 03:14:41 nas kernel: ata1.00: failed command: WRITE FPDMA
>> QUEUED
>
> But these WILL cause incorrect data written to disk, in my experience.
> After
> that, one of your disks will contain some corruption, whether in files,
> or (as
> you discovered) in the filesystem itself.
This is the "scary" part: if the write was not acknowledged as committed
to disk, why the block layer did not report it to the MD driver? Or if
the block layer reported that, why MD did not kick the disk out of the
array?
> mdadm may or may not read from that
> disk, as it chooses the mirror for reads pretty much randomly, using
> the least
> loaded one. And even though the other disk still contains good data,
> there is
> no mechanism for the user-space to say "hey, this doesn't look right,
> what's
> on the other mirror?"
I understand and agree with that. I'm fully aware that MD can not (by
design) detect/correct corrupted data. However, I wonder if, and why, a
disk with obvious errors was not kicked out of the array.
>
> Check your cables and/or disks themselves.
>
I tried reseating and inverting the cables ;)
Let see if the problem disappears or if it "follow" the
cable/drive/interface...
> If you know that only one disk had these write errors all the time, you
> could
> try disconnecting it from mirror, and checking if you can get a more
> consistent view of the filesystem on the remaining one.
>
> P.S: about my case (which I witnessed on a RAID6):
>
> * copy a file to the array, one disk will hit tons of WRITE FPDMA
> QUEUED
> errors (due to insufficient power and/or bad data cable).
> * the file that was just copied, turns out to be corrupted when
> reading back.
> * the problem disk WILL NOT get kicked from the array during this.
Wow, a die-hard data corruption. It seems VERY similar to what happened
to me, and the key problem seems the same: a failing drive was not
detached from the array in a timely fashion.
Thanks very much for reporting, Roman.
--
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8
^ permalink raw reply
* Re: Filesystem corruption on RAID1
From: Reindl Harald @ 2017-07-13 21:34 UTC (permalink / raw)
To: Gionatan Danti, Roman Mamedov; +Cc: linux-raid
In-Reply-To: <592f19bf608e9a959f9445f7f25c5dad@assyoma.it>
Am 13.07.2017 um 23:28 schrieb Gionatan Danti:
> I understand and agree with that. I'm fully aware that MD can not (by
> design) detect/correct corrupted data. However, I wonder if, and why, a
> disk with obvious errors was not kicked out of the array.
maybe because the disk is, well, not in a good shape and don't know that
by itself - i had storage devices which refused to write but said
nothing (flash media), frankly you where able to even format that crap
and overwrite if with zeros and all looked fine - until you pulled the
broken device and inserted it again - same data as yesterday - a sd-card
doestroyed a smartphone phisically by empty the whole battey within 30
minutes while sitting in the cinema
broken hardware don't know that it's broken moste of the time
that#s why you need always backups or can just delete the data at all
because they are not important
thins like above only could be detected by verify every write with an
uncached read/verify which would lead in a uneccaptable performane
penalty (and no filesystems with checksums won't magically recover your
data, they just tell you realier they are gone)
^ permalink raw reply
* Re: [PATCH 2/2] md: raid1/raid10: initialize bvec table via bio_add_page()
From: NeilBrown @ 2017-07-13 22:08 UTC (permalink / raw)
To: Shaohua Li, Ming Lei
Cc: linux-raid, linux-block, Jens Axboe, Christoph Hellwig
In-Reply-To: <20170713163909.whltvlms2zwgevkf@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 4376 bytes --]
On Thu, Jul 13 2017, Shaohua Li wrote:
> On Thu, Jul 13, 2017 at 05:20:52PM +0800, Ming Lei wrote:
>> On Thu, Jul 13, 2017 at 01:09:28PM +1000, NeilBrown wrote:
>> > On Thu, Jul 13 2017, Ming Lei wrote:
>> >
>> > > On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote:
>> > >> On Wed, Jul 12 2017, Ming Lei wrote:
>> > >>
>> > >> > We will support multipage bvec soon, so initialize bvec
>> > >> > table using the standardy way instead of writing the
>> > >> > talbe directly. Otherwise it won't work any more once
>> > >> > multipage bvec is enabled.
>> > >> >
>> > >> > Acked-by: Guoqing Jiang <gqjiang@suse.com>
>> > >> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> > >> > ---
>> > >> > drivers/md/md.c | 21 +++++++++++++++++++++
>> > >> > drivers/md/md.h | 3 +++
>> > >> > drivers/md/raid1.c | 16 ++--------------
>> > >> > drivers/md/raid10.c | 4 ++--
>> > >> > 4 files changed, 28 insertions(+), 16 deletions(-)
>> > >> >
>> > >> > diff --git a/drivers/md/md.c b/drivers/md/md.c
>> > >> > index 8cdca0296749..cc8dcd928dde 100644
>> > >> > --- a/drivers/md/md.c
>> > >> > +++ b/drivers/md/md.c
>> > >> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr)
>> > >> > }
>> > >> > EXPORT_SYMBOL(md_reload_sb);
>> > >> >
>> > >> > +/* generally called after bio_reset() for reseting bvec */
>> > >> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
>> > >> > + int size)
>> > >> > +{
>> > >> > + int idx = 0;
>> > >> > +
>> > >> > + /* initialize bvec table again */
>> > >> > + do {
>> > >> > + struct page *page = resync_fetch_page(rp, idx);
>> > >> > + int len = min_t(int, size, PAGE_SIZE);
>> > >> > +
>> > >> > + /*
>> > >> > + * won't fail because the vec table is big
>> > >> > + * enough to hold all these pages
>> > >> > + */
>> > >> > + bio_add_page(bio, page, len, 0);
>> > >> > + size -= len;
>> > >> > + } while (idx++ < RESYNC_PAGES && size > 0);
>> > >> > +}
>> > >> > +EXPORT_SYMBOL(md_bio_reset_resync_pages);
>> > >>
>> > >> I really don't think this is a good idea.
>> > >> This code is specific to raid1/raid10. It is not generic md code. So
>> > >> it doesn't belong here.
>> > >
>> > > We already added 'struct resync_pages' in drivers/md/md.h, so I think
>> > > it is reasonable to add this function into drivers/md/md.c
>> >
>> > Alternative perspective: it was unreasonable to add "resync_pages" to
>> > md.h.
>> >
>> > >
>> > >>
>> > >> If you want to remove code duplication, then work on moving all raid1
>> > >> functionality into raid10.c, then discard raid1.c
>> > >
>> > > This patch is for avoiding new code duplication, not for removing current
>> > > duplication.
>> > >
>> > >>
>> > >> Or at the very least, have a separate "raid1-10.c" file for the common
>> > >> code.
>> > >
>> > > You suggested it last time, but looks too overkill to be taken. But if
>> > > someone wants to refactor raid1 and raid10, I think it can be a good start,
>> > > but still not belong to this patch.
>> >
>> > You are trying to create common code for raid1 and raid10. This does
>> > not belong in md.c.
>> > If you really want to have a single copy of common code, then it exactly
>> > is the role of this patch to create a place to put it.
>> > I'm not saying you should put all common code in raid1-10.c. Just the
>> > function that you have identified.
>>
>> I really don't want to waste time on this kind of thing, I can do
>> either one frankly.
>>
>> Shaohua, could you share me which way you like to merge? I can do it in
>> either way.
>
> I don't have strong preference, but Neil's suggestion does make the code a
> little better. Of course, only put the function into the raid1-10.c right now.
To make it as easy as possible, I would suggest creating raid1-10.c
containing just this function (and maybe the definitions from md.h),
and declare the function "static" and #include raid1-10.c into raid1.c
and raid10.c. i.e. no worrying about modules and exporting symbols.
Anyone who cares (and that might even be me) could move functionality
gradually out of raid1.c and raid10.c in raid1-10.c. Maybe where would
come a tipping-point where it is easy to just discard raid1.c and raid10.c
and finish the job.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox