On Thu, Feb 16, 2017 at 12:35:22AM +0800, colyli@xxxxxxx wrote:
> 'Commit 79ef3a8aa1cb ("raid1: Rewrite the implementation of iobarrier.")'
> introduces a sliding resync window for raid1 I/O barrier, this idea limits
> I/O barriers to happen only inside a slidingresync window, for regular
> I/Os out of this resync window they don't need to wait for barrier any
> more. On large raid1 device, it helps a lot to improve parallel writing
> I/O throughput when there are background resync I/Os performing at
> same time.
>
> The idea of sliding resync widow is awesome, but code complexity is a
> challenge. Sliding resync window requires several veriables to work
> collectively, this is complexed and very hard to make it work correctly.
> Just grep "Fixes: 79ef3a8aa1" in kernel git log, there are 8 more patches
> to fix the original resync window patch. This is not the end, any further
> related modification may easily introduce more regreassion.
>
> Therefore I decide to implement a much simpler raid1 I/O barrier, by
> removing resync window code, I believe life will be much easier.
>
> The brief idea of the simpler barrier is,
> - Do not maintain a logbal unique resync window
> - Use multiple hash buckets to reduce I/O barrier conflictions, regular
> I/O only has to wait for a resync I/O when both them have same barrier
> bucket index, vice versa.
> - I/O barrier can be recuded to an acceptable number if there are enought
> barrier buckets
>
> Here I explain how the barrier buckets are designed,
> - BARRIER_UNIT_SECTOR_SIZE
> The whole LBA address space of a raid1 device is divided into multiple
> barrier units, by the size of BARRIER_UNIT_SECTOR_SIZE.
> Bio request won't go across border of barrier unit size, that means
> maximum bio size is BARRIER_UNIT_SECTOR_SIZE<<9 (64MB) in bytes.
> For random I/O 64MB is large enough for both read and write requests,
> for sequential I/O considering underlying block layer may merge them
> into larger requests, 64MB is still good enough.
> Neil also points out that for resync operation, "we want the resync to
> move from region to region fairly quickly so that the slowness caused
> by having to synchronize with the resync is averaged out over a fairly
> small time frame". For full speed resync, 64MB should take less then 1
> second. When resync is competing with other I/O, it could take up a few
> minutes. Therefore 64MB size is fairly good range for resync.
>
> - BARRIER_BUCKETS_NR
> There are BARRIER_BUCKETS_NR buckets in total, which is defined by,
> #define BARRIER_BUCKETS_NR_BITS (PAGE_SHIFT - 2)
> #define BARRIER_BUCKETS_NR (1<<BARRIER_BUCKETS_NR_BITS)
> this patch makes the bellowed members of struct r1conf from integer
> to array of integers,
> - int nr_pending;
> - int nr_waiting;
> - int nr_queued;
> - int barrier;
> + int *nr_pending;
> + int *nr_waiting;
> + int *nr_queued;
> + int *barrier;
> number of the array elements is defined as BARRIER_BUCKETS_NR. For 4KB
> kernel space page size, (PAGE_SHIFT - 2) indecates there are 1024 I/O
> barrier buckets, and each array of integers occupies single memory page.
> 1024 means for a request which is smaller than the I/O barrier unit size
> has ~0.1% chance to wait for resync to pause, which is quite a small
> enough fraction. Also requesting single memory page is more friendly to
> kernel page allocator than larger memory size.
>
> - I/O barrier bucket is indexed by bio start sector
> If multiple I/O requests hit different I/O barrier units, they only need
> to compete I/O barrier with other I/Os which hit the same I/O barrier
> bucket index with each other. The index of a barrier bucket which a
> bio should look for is calculated by sector_to_idx() which is defined
> in raid1.h as an inline function,
> static inline int sector_to_idx(sector_t sector)
> {
> return hash_long(sector >> BARRIER_UNIT_SECTOR_BITS,
> BARRIER_BUCKETS_NR_BITS);
> }
> Here sector_nr is the start sector number of a bio.
>
> - Single bio won't go across boundary of a I/O barrier unit
> If a request goes across boundary of barrier unit, it will be split. A
> bio may be split in raid1_make_request() or raid1_sync_request(), if
> sectors returned by align_to_barrier_unit_end() is small than original
> bio size.
>
> Comparing to single sliding resync window,
> - Currently resync I/O grows linearly, therefore regular and resync I/O
> will have confliction within a single barrier units. So the I/O
> behavior is similar to single sliding resync window.
> - But a barrier unit bucket is shared by all barrier units with identical
> barrier uinit index, the probability of confliction might be higher
> than single sliding resync window, in condition that writing I/Os
> always hit barrier units which have identical barrier bucket indexs with
> the resync I/Os. This is a very rare condition in real I/O work loads,
> I cannot imagine how it could happen in practice.
> - Therefore we can achieve a good enough low confliction rate with much
> simpler barrier algorithm and implementation.
>
> There are two changes should be noticed,
> - In raid1d(), I change the code to decrease conf->nr_pending[idx] into
> single loop, it looks like this,
> spin_lock_irqsave(&conf->device_lock, flags);
> conf->nr_queued[idx]--;
> spin_unlock_irqrestore(&conf->device_lock, flags);
> This change generates more spin lock operations, but in next patch of
> this patch set, it will be replaced by a single line code,
> atomic_dec(&conf->nr_queueud[idx]);
> So we don't need to worry about spin lock cost here.
> - Mainline raid1 code split original raid1_make_request() into
> raid1_read_request() and raid1_write_request(). If the original bio
> goes across an I/O barrier unit size, this bio will be split before
> calling raid1_read_request() or raid1_write_request(), this change
> the code logic more simple and clear.
> - In this patch wait_barrier() is moved from raid1_make_request() to
> raid1_write_request(). In raid_read_request(), original wait_barrier()
> is replaced by raid1_read_request().
> The differnece is wait_read_barrier() only waits if array is frozen,
> using different barrier function in different code path makes the code
> more clean and easy to read.
> Changelog
> V3:
> - Rebase the patch against latest upstream kernel code.
> - Many fixes by review comments from Neil,
> - Back to use pointers to replace arraries in struct r1conf
> - Remove total_barriers from struct r1conf
> - Add more patch comments to explain how/why the values of
> BARRIER_UNIT_SECTOR_SIZE and BARRIER_BUCKETS_NR are decided.
> - Use get_unqueued_pending() to replace get_all_pendings() and
> get_all_queued()
> - Increase bucket number from 512 to 1024
> - Change code comments format by review from Shaohua.
> V2:
> - Use bio_split() to split the orignal bio if it goes across barrier unit
> bounday, to make the code more simple, by suggestion from Shaohua and
> Neil.
> - Use hash_long() to replace original linear hash, to avoid a possible
> confilict between resync I/O and sequential write I/O, by suggestion from
> Shaohua.
> - Add conf->total_barriers to record barrier depth, which is used to
> control number of parallel sync I/O barriers, by suggestion from Shaohua.
> - In V1 patch the bellowed barrier buckets related members in r1conf are
> allocated in memory page. To make the code more simple, V2 patch moves
> the memory space into struct r1conf, like this,
> - int nr_pending;
> - int nr_waiting;
> - int nr_queued;
> - int barrier;
> + int nr_pending[BARRIER_BUCKETS_NR];
> + int nr_waiting[BARRIER_BUCKETS_NR];
> + int nr_queued[BARRIER_BUCKETS_NR];
> + int barrier[BARRIER_BUCKETS_NR];
> This change is by the suggestion from Shaohua.
> - Remove some inrelavent code comments, by suggestion from Guoqing.
> - Add a missing wait_barrier() before jumping to retry_write, in
> raid1_make_write_request().
> V1:
> - Original RFC patch for comments
Looks good, two minor issues.
>
> -static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> - struct r1bio *r1_bio)
> +static void raid1_read_request(struct mddev *mddev, struct bio *bio)
> {
> struct r1conf *conf = mddev->private;
> struct raid1_info *mirror;
> + struct r1bio *r1_bio;
> struct bio *read_bio;
> struct bitmap *bitmap = mddev->bitmap;
> const int op = bio_op(bio);
> @@ -1083,7 +1101,34 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> int max_sectors;
> int rdisk;
>
> - wait_barrier(conf, bio);
> + /*
> + * Still need barrier for READ in case that whole
> + * array is frozen.
> + */
> + wait_read_barrier(conf, bio->bi_iter.bi_sector);
> + bitmap = mddev->bitmap;
> +
> + /*
> + * make_request() can abort the operation when read-ahead is being
> + * used and no empty request is available.
> + *
> + */
> + r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
> + r1_bio->master_bio = bio;
> + r1_bio->sectors = bio_sectors(bio);
> + r1_bio->state = 0;
> + r1_bio->mddev = mddev;
> + r1_bio->sector = bio->bi_iter.bi_sector;
This part looks unnecessary complicated. If you change raid1_make_request to
something like __raid1_make_reques, add a new raid1_make_request and do bio
split there, then call __raid1_make_request for each splitted bio, then you
don't need to duplicate the r1_bio allocation parts for read/write.
> diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
> index c52ef42..d3faf30 100644
> --- a/drivers/md/raid1.h
> +++ b/drivers/md/raid1.h
> @@ -1,6 +1,14 @@
> #ifndef _RAID1_H
> #define _RAID1_H
>
> +/* each barrier unit size is 64MB fow now
> + * note: it must be larger than RESYNC_DEPTH
> + */
> +#define BARRIER_UNIT_SECTOR_BITS 17
> +#define BARRIER_UNIT_SECTOR_SIZE (1<<17)
> +#define BARRIER_BUCKETS_NR_BITS (PAGE_SHIFT - 2)
maybe write this as (PAGE_SHIFT - ilog2(sizeof(int)))? To be honest, I don't
think it really matters if the array is PAGE_SIZE length, maybe just specify a
const here.
Thanks,
Shaohua
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
