From: Christoph Hellwig <hch@infradead.org> Date: 2021-08-13 07:50:12
On Fri, Aug 13, 2021 at 02:05:10PM +0800, Guoqing Jiang wrote:
From: Guoqing Jiang <redacted>
We can't split bio with more than BIO_MAX_VECS sectors, otherwise the
below call trace was triggered because we could allocate oversized
write behind bio later.
[ 8.097936] bvec_alloc+0x90/0xc0
[ 8.098934] bio_alloc_bioset+0x1b3/0x260
[ 8.099959] raid1_make_request+0x9ce/0xc50 [raid1]
Which bio_alloc_bioset is this? The one in alloc_behind_master_bio?
In which case I think you want to limit the reduction of max_sectors
to just the write behind case, and clearly document what is going on.
In general the size of a bio only depends on the number of vectors, not
the total I/O size. But alloc_behind_master_bio allocates new backing
pages using order 0 allocations, so in this exceptional case the total
size oes actually matter.
While we're at it: this huge memory allocation looks really deadlock
prone.
On Fri, Aug 13, 2021 at 02:05:10PM +0800, Guoqing Jiang wrote:
quoted
From: Guoqing Jiang <redacted>
We can't split bio with more than BIO_MAX_VECS sectors, otherwise the
below call trace was triggered because we could allocate oversized
write behind bio later.
[ 8.097936] bvec_alloc+0x90/0xc0
[ 8.098934] bio_alloc_bioset+0x1b3/0x260
[ 8.099959] raid1_make_request+0x9ce/0xc50 [raid1]
Which bio_alloc_bioset is this? The one in alloc_behind_master_bio?
Yes, it should be the one since bio_clone_fast calls bio_alloc_bioset
with 0 iovecs.
In which case I think you want to limit the reduction of max_sectors
to just the write behind case, and clearly document what is going on.
Ok, thanks.
In general the size of a bio only depends on the number of vectors, not
the total I/O size. But alloc_behind_master_bio allocates new backing
pages using order 0 allocations, so in this exceptional case the total
size oes actually matter.
While we're at it: this huge memory allocation looks really deadlock
prone.
Hmm, let me think more about it, or could you share your thought? 😉
Thanks,
Guoqing
From: kernel test robot <hidden> Date: 2021-08-13 09:28:01
Hi Guoqing,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on song-md/md-next]
[also build test ERROR on v5.14-rc5 next-20210812]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Guoqing-Jiang/raid1-ensure-bio-doesn-t-have-more-than-BIO_MAX_VECS-sectors/20210813-140810
base: git://git.kernel.org/pub/scm/linux/kernel/git/song/md.git md-next
config: hexagon-randconfig-r001-20210813 (attached as .config)
compiler: clang version 12.0.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/29b7720a83de1deea0d8ecfafe0db46146636b15
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guoqing-Jiang/raid1-ensure-bio-doesn-t-have-more-than-BIO_MAX_VECS-sectors/20210813-140810
git checkout 29b7720a83de1deea0d8ecfafe0db46146636b15
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/md/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>
All errors (new ones prefixed by >>):
quoted
drivers/md/raid1.c:1459:55: error: use of undeclared identifier 'PAGE_SECTORS'
drivers/md/raid1.c:1459:55: error: use of undeclared identifier 'PAGE_SECTORS'
2 errors generated.
vim +/PAGE_SECTORS +1459 drivers/md/raid1.c
1320
1321 static void raid1_write_request(struct mddev *mddev, struct bio *bio,
1322 int max_write_sectors)
1323 {
1324 struct r1conf *conf = mddev->private;
1325 struct r1bio *r1_bio;
1326 int i, disks;
1327 struct bitmap *bitmap = mddev->bitmap;
1328 unsigned long flags;
1329 struct md_rdev *blocked_rdev;
1330 struct blk_plug_cb *cb;
1331 struct raid1_plug_cb *plug = NULL;
1332 int first_clone;
1333 int max_sectors;
1334
1335 if (mddev_is_clustered(mddev) &&
1336 md_cluster_ops->area_resyncing(mddev, WRITE,
1337 bio->bi_iter.bi_sector, bio_end_sector(bio))) {
1338
1339 DEFINE_WAIT(w);
1340 for (;;) {
1341 prepare_to_wait(&conf->wait_barrier,
1342 &w, TASK_IDLE);
1343 if (!md_cluster_ops->area_resyncing(mddev, WRITE,
1344 bio->bi_iter.bi_sector,
1345 bio_end_sector(bio)))
1346 break;
1347 schedule();
1348 }
1349 finish_wait(&conf->wait_barrier, &w);
1350 }
1351
1352 /*
1353 * Register the new request and wait if the reconstruction
1354 * thread has put up a bar for new requests.
1355 * Continue immediately if no resync is active currently.
1356 */
1357 wait_barrier(conf, bio->bi_iter.bi_sector);
1358
1359 r1_bio = alloc_r1bio(mddev, bio);
1360 r1_bio->sectors = max_write_sectors;
1361
1362 if (conf->pending_count >= max_queued_requests) {
1363 md_wakeup_thread(mddev->thread);
1364 raid1_log(mddev, "wait queued");
1365 wait_event(conf->wait_barrier,
1366 conf->pending_count < max_queued_requests);
1367 }
1368 /* first select target devices under rcu_lock and
1369 * inc refcount on their rdev. Record them by setting
1370 * bios[x] to bio
1371 * If there are known/acknowledged bad blocks on any device on
1372 * which we have seen a write error, we want to avoid writing those
1373 * blocks.
1374 * This potentially requires several writes to write around
1375 * the bad blocks. Each set of writes gets it's own r1bio
1376 * with a set of bios attached.
1377 */
1378
1379 disks = conf->raid_disks * 2;
1380 retry_write:
1381 blocked_rdev = NULL;
1382 rcu_read_lock();
1383 max_sectors = r1_bio->sectors;
1384 for (i = 0; i < disks; i++) {
1385 struct md_rdev *rdev = rcu_dereference(conf->mirrors[i].rdev);
1386 if (rdev && unlikely(test_bit(Blocked, &rdev->flags))) {
1387 atomic_inc(&rdev->nr_pending);
1388 blocked_rdev = rdev;
1389 break;
1390 }
1391 r1_bio->bios[i] = NULL;
1392 if (!rdev || test_bit(Faulty, &rdev->flags)) {
1393 if (i < conf->raid_disks)
1394 set_bit(R1BIO_Degraded, &r1_bio->state);
1395 continue;
1396 }
1397
1398 atomic_inc(&rdev->nr_pending);
1399 if (test_bit(WriteErrorSeen, &rdev->flags)) {
1400 sector_t first_bad;
1401 int bad_sectors;
1402 int is_bad;
1403
1404 is_bad = is_badblock(rdev, r1_bio->sector, max_sectors,
1405 &first_bad, &bad_sectors);
1406 if (is_bad < 0) {
1407 /* mustn't write here until the bad block is
1408 * acknowledged*/
1409 set_bit(BlockedBadBlocks, &rdev->flags);
1410 blocked_rdev = rdev;
1411 break;
1412 }
1413 if (is_bad && first_bad <= r1_bio->sector) {
1414 /* Cannot write here at all */
1415 bad_sectors -= (r1_bio->sector - first_bad);
1416 if (bad_sectors < max_sectors)
1417 /* mustn't write more than bad_sectors
1418 * to other devices yet
1419 */
1420 max_sectors = bad_sectors;
1421 rdev_dec_pending(rdev, mddev);
1422 /* We don't set R1BIO_Degraded as that
1423 * only applies if the disk is
1424 * missing, so it might be re-added,
1425 * and we want to know to recover this
1426 * chunk.
1427 * In this case the device is here,
1428 * and the fact that this chunk is not
1429 * in-sync is recorded in the bad
1430 * block log
1431 */
1432 continue;
1433 }
1434 if (is_bad) {
1435 int good_sectors = first_bad - r1_bio->sector;
1436 if (good_sectors < max_sectors)
1437 max_sectors = good_sectors;
1438 }
1439 }
1440 r1_bio->bios[i] = bio;
1441 }
1442 rcu_read_unlock();
1443
1444 if (unlikely(blocked_rdev)) {
1445 /* Wait for this device to become unblocked */
1446 int j;
1447
1448 for (j = 0; j < i; j++)
1449 if (r1_bio->bios[j])
1450 rdev_dec_pending(conf->mirrors[j].rdev, mddev);
1451 r1_bio->state = 0;
1452 allow_barrier(conf, bio->bi_iter.bi_sector);
1453 raid1_log(mddev, "wait rdev %d blocked", blocked_rdev->raid_disk);
1454 md_wait_for_blocked_rdev(blocked_rdev, mddev);
1455 wait_barrier(conf, bio->bi_iter.bi_sector);
1456 goto retry_write;
1457 }
1458
From: kernel test robot <hidden> Date: 2021-08-13 10:13:10
Hi Guoqing,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on song-md/md-next]
[also build test ERROR on v5.14-rc5 next-20210812]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Guoqing-Jiang/raid1-ensure-bio-doesn-t-have-more-than-BIO_MAX_VECS-sectors/20210813-140810
base: git://git.kernel.org/pub/scm/linux/kernel/git/song/md.git md-next
config: nds32-randconfig-r035-20210813 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/29b7720a83de1deea0d8ecfafe0db46146636b15
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guoqing-Jiang/raid1-ensure-bio-doesn-t-have-more-than-BIO_MAX_VECS-sectors/20210813-140810
git checkout 29b7720a83de1deea0d8ecfafe0db46146636b15
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=nds32 SHELL=/bin/bash drivers/md/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>
All errors (new ones prefixed by >>):
In file included from include/linux/kernel.h:15,
from include/asm-generic/bug.h:20,
from ./arch/nds32/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/gfp.h:5,
from include/linux/slab.h:15,
from drivers/md/raid1.c:26:
drivers/md/raid1.c: In function 'raid1_write_request':
quoted
drivers/md/raid1.c:1459:55: error: 'PAGE_SECTORS' undeclared (first use in this function); did you mean 'PAGE_MEMORY'?
1459 | max_sectors = min_t(int, max_sectors, BIO_MAX_VECS * PAGE_SECTORS);
| ^~~~~~~~~~~~
include/linux/minmax.h:20:39: note: in definition of macro '__typecheck'
20 | (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
| ^
include/linux/minmax.h:36:24: note: in expansion of macro '__safe_cmp'
36 | __builtin_choose_expr(__safe_cmp(x, y), \
| ^~~~~~~~~~
include/linux/minmax.h:104:27: note: in expansion of macro '__careful_cmp'
104 | #define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
| ^~~~~~~~~~~~~
drivers/md/raid1.c:1459:16: note: in expansion of macro 'min_t'
1459 | max_sectors = min_t(int, max_sectors, BIO_MAX_VECS * PAGE_SECTORS);
| ^~~~~
drivers/md/raid1.c:1459:55: note: each undeclared identifier is reported only once for each function it appears in
1459 | max_sectors = min_t(int, max_sectors, BIO_MAX_VECS * PAGE_SECTORS);
| ^~~~~~~~~~~~
include/linux/minmax.h:20:39: note: in definition of macro '__typecheck'
20 | (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
| ^
include/linux/minmax.h:36:24: note: in expansion of macro '__safe_cmp'
36 | __builtin_choose_expr(__safe_cmp(x, y), \
| ^~~~~~~~~~
include/linux/minmax.h:104:27: note: in expansion of macro '__careful_cmp'
104 | #define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
| ^~~~~~~~~~~~~
drivers/md/raid1.c:1459:16: note: in expansion of macro 'min_t'
1459 | max_sectors = min_t(int, max_sectors, BIO_MAX_VECS * PAGE_SECTORS);
| ^~~~~
quoted
include/linux/minmax.h:36:2: error: first argument to '__builtin_choose_expr' not a constant
36 | __builtin_choose_expr(__safe_cmp(x, y), \
| ^~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:104:27: note: in expansion of macro '__careful_cmp'
104 | #define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
| ^~~~~~~~~~~~~
drivers/md/raid1.c:1459:16: note: in expansion of macro 'min_t'
1459 | max_sectors = min_t(int, max_sectors, BIO_MAX_VECS * PAGE_SECTORS);
| ^~~~~
vim +1459 drivers/md/raid1.c
1320
1321 static void raid1_write_request(struct mddev *mddev, struct bio *bio,
1322 int max_write_sectors)
1323 {
1324 struct r1conf *conf = mddev->private;
1325 struct r1bio *r1_bio;
1326 int i, disks;
1327 struct bitmap *bitmap = mddev->bitmap;
1328 unsigned long flags;
1329 struct md_rdev *blocked_rdev;
1330 struct blk_plug_cb *cb;
1331 struct raid1_plug_cb *plug = NULL;
1332 int first_clone;
1333 int max_sectors;
1334
1335 if (mddev_is_clustered(mddev) &&
1336 md_cluster_ops->area_resyncing(mddev, WRITE,
1337 bio->bi_iter.bi_sector, bio_end_sector(bio))) {
1338
1339 DEFINE_WAIT(w);
1340 for (;;) {
1341 prepare_to_wait(&conf->wait_barrier,
1342 &w, TASK_IDLE);
1343 if (!md_cluster_ops->area_resyncing(mddev, WRITE,
1344 bio->bi_iter.bi_sector,
1345 bio_end_sector(bio)))
1346 break;
1347 schedule();
1348 }
1349 finish_wait(&conf->wait_barrier, &w);
1350 }
1351
1352 /*
1353 * Register the new request and wait if the reconstruction
1354 * thread has put up a bar for new requests.
1355 * Continue immediately if no resync is active currently.
1356 */
1357 wait_barrier(conf, bio->bi_iter.bi_sector);
1358
1359 r1_bio = alloc_r1bio(mddev, bio);
1360 r1_bio->sectors = max_write_sectors;
1361
1362 if (conf->pending_count >= max_queued_requests) {
1363 md_wakeup_thread(mddev->thread);
1364 raid1_log(mddev, "wait queued");
1365 wait_event(conf->wait_barrier,
1366 conf->pending_count < max_queued_requests);
1367 }
1368 /* first select target devices under rcu_lock and
1369 * inc refcount on their rdev. Record them by setting
1370 * bios[x] to bio
1371 * If there are known/acknowledged bad blocks on any device on
1372 * which we have seen a write error, we want to avoid writing those
1373 * blocks.
1374 * This potentially requires several writes to write around
1375 * the bad blocks. Each set of writes gets it's own r1bio
1376 * with a set of bios attached.
1377 */
1378
1379 disks = conf->raid_disks * 2;
1380 retry_write:
1381 blocked_rdev = NULL;
1382 rcu_read_lock();
1383 max_sectors = r1_bio->sectors;
1384 for (i = 0; i < disks; i++) {
1385 struct md_rdev *rdev = rcu_dereference(conf->mirrors[i].rdev);
1386 if (rdev && unlikely(test_bit(Blocked, &rdev->flags))) {
1387 atomic_inc(&rdev->nr_pending);
1388 blocked_rdev = rdev;
1389 break;
1390 }
1391 r1_bio->bios[i] = NULL;
1392 if (!rdev || test_bit(Faulty, &rdev->flags)) {
1393 if (i < conf->raid_disks)
1394 set_bit(R1BIO_Degraded, &r1_bio->state);
1395 continue;
1396 }
1397
1398 atomic_inc(&rdev->nr_pending);
1399 if (test_bit(WriteErrorSeen, &rdev->flags)) {
1400 sector_t first_bad;
1401 int bad_sectors;
1402 int is_bad;
1403
1404 is_bad = is_badblock(rdev, r1_bio->sector, max_sectors,
1405 &first_bad, &bad_sectors);
1406 if (is_bad < 0) {
1407 /* mustn't write here until the bad block is
1408 * acknowledged*/
1409 set_bit(BlockedBadBlocks, &rdev->flags);
1410 blocked_rdev = rdev;
1411 break;
1412 }
1413 if (is_bad && first_bad <= r1_bio->sector) {
1414 /* Cannot write here at all */
1415 bad_sectors -= (r1_bio->sector - first_bad);
1416 if (bad_sectors < max_sectors)
1417 /* mustn't write more than bad_sectors
1418 * to other devices yet
1419 */
1420 max_sectors = bad_sectors;
1421 rdev_dec_pending(rdev, mddev);
1422 /* We don't set R1BIO_Degraded as that
1423 * only applies if the disk is
1424 * missing, so it might be re-added,
1425 * and we want to know to recover this
1426 * chunk.
1427 * In this case the device is here,
1428 * and the fact that this chunk is not
1429 * in-sync is recorded in the bad
1430 * block log
1431 */
1432 continue;
1433 }
1434 if (is_bad) {
1435 int good_sectors = first_bad - r1_bio->sector;
1436 if (good_sectors < max_sectors)
1437 max_sectors = good_sectors;
1438 }
1439 }
1440 r1_bio->bios[i] = bio;
1441 }
1442 rcu_read_unlock();
1443
1444 if (unlikely(blocked_rdev)) {
1445 /* Wait for this device to become unblocked */
1446 int j;
1447
1448 for (j = 0; j < i; j++)
1449 if (r1_bio->bios[j])
1450 rdev_dec_pending(conf->mirrors[j].rdev, mddev);
1451 r1_bio->state = 0;
1452 allow_barrier(conf, bio->bi_iter.bi_sector);
1453 raid1_log(mddev, "wait rdev %d blocked", blocked_rdev->raid_disk);
1454 md_wait_for_blocked_rdev(blocked_rdev, mddev);
1455 wait_barrier(conf, bio->bi_iter.bi_sector);
1456 goto retry_write;
1457 }
1458
From: Christoph Hellwig <hch@infradead.org> Date: 2021-08-14 07:56:21
On Fri, Aug 13, 2021 at 04:38:59PM +0800, Guoqing Jiang wrote:
Ok, thanks.
quoted
In general the size of a bio only depends on the number of vectors, not
the total I/O size. But alloc_behind_master_bio allocates new backing
pages using order 0 allocations, so in this exceptional case the total
size oes actually matter.
While we're at it: this huge memory allocation looks really deadlock
prone.
Hmm, let me think more about it, or could you share your thought? ????
Well, you'd need a mempool which can fit the max payload of a bio,
that is BIO_MAX_VECS pages.
FYI, this is what I'd do instead of this patch for now. We don't really
need a vetor per sector, just per page. So this limits the I/O
size a little less.
On Sat, Aug 14, 2021 at 08:55:21AM +0100, Christoph Hellwig wrote:
quoted hunk
On Fri, Aug 13, 2021 at 04:38:59PM +0800, Guoqing Jiang wrote:
quoted
Ok, thanks.
quoted
In general the size of a bio only depends on the number of vectors, not
the total I/O size. But alloc_behind_master_bio allocates new backing
pages using order 0 allocations, so in this exceptional case the total
size oes actually matter.
While we're at it: this huge memory allocation looks really deadlock
prone.
Hmm, let me think more about it, or could you share your thought? ????
Well, you'd need a mempool which can fit the max payload of a bio,
that is BIO_MAX_VECS pages.
FYI, this is what I'd do instead of this patch for now. We don't really
need a vetor per sector, just per page. So this limits the I/O
size a little less.
+
if (max_sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, max_sectors,
GFP_NOIO, &conf->bio_split);
Here the limit is max single-page vectors, and the above way may not work,
such as:
0 ~ 254: each bvec's length is 512
255: bvec's length is 8192
the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().
One solution is to add queue limit of max_single_page_bvec, and let
blk_queue_split() handle it.
Thanks,
Ming
Hi Ming and Christoph,
On 8/14/21 4:57 PM, Ming Lei wrote:
On Sat, Aug 14, 2021 at 08:55:21AM +0100, Christoph Hellwig wrote:
quoted
On Fri, Aug 13, 2021 at 04:38:59PM +0800, Guoqing Jiang wrote:
quoted
Ok, thanks.
quoted
In general the size of a bio only depends on the number of vectors, not
the total I/O size. But alloc_behind_master_bio allocates new backing
pages using order 0 allocations, so in this exceptional case the total
size oes actually matter.
While we're at it: this huge memory allocation looks really deadlock
prone.
Hmm, let me think more about it, or could you share your thought? ????
Well, you'd need a mempool which can fit the max payload of a bio,
that is BIO_MAX_VECS pages.
IIUC, the behind bio is allocated from bio_set (mddev->bio_set) which is
allocated in md_run by
call bioset_init, so the mempool (bvec_pool) of this bio_set is created
by biovec_init_pool which
uses global biovec slabs. Do we really need another mempool? Or, there
is no potential deadlock
for this case.
quoted
FYI, this is what I'd do instead of this patch for now. We don't really
need a vetor per sector, just per page. So this limits the I/O
size a little less.
+
if (max_sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, max_sectors,
GFP_NOIO, &conf->bio_split);
Here the limit is max single-page vectors, and the above way may not work,
such as:ust splitted and not
0 ~ 254: each bvec's length is 512
255: bvec's length is 8192
the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().
Thanks for deeper looking! I guess it is because how vcnt is calculated.
One solution is to add queue limit of max_single_page_bvec, and let
blk_queue_split() handle it.
The path (blk_queue_split -> blk_bio_segment_split -> bvec_split_segs)
which respects max_segments
of limit. Do you mean introduce max_single_page_bvec to limit? Then
perform similar checking as for
max_segment.
Thanks,
Guoqing
On Mon, Aug 16, 2021 at 02:27:48PM +0800, Guoqing Jiang wrote:
Hi Ming and Christoph,
On 8/14/21 4:57 PM, Ming Lei wrote:
quoted
On Sat, Aug 14, 2021 at 08:55:21AM +0100, Christoph Hellwig wrote:
quoted
On Fri, Aug 13, 2021 at 04:38:59PM +0800, Guoqing Jiang wrote:
quoted
Ok, thanks.
quoted
In general the size of a bio only depends on the number of vectors, not
the total I/O size. But alloc_behind_master_bio allocates new backing
pages using order 0 allocations, so in this exceptional case the total
size oes actually matter.
While we're at it: this huge memory allocation looks really deadlock
prone.
Hmm, let me think more about it, or could you share your thought? ????
Well, you'd need a mempool which can fit the max payload of a bio,
that is BIO_MAX_VECS pages.
IIUC, the behind bio is allocated from bio_set (mddev->bio_set) which is
allocated in md_run by
call bioset_init, so the mempool (bvec_pool) of this bio_set is created by
biovec_init_pool which
uses global biovec slabs. Do we really need another mempool? Or, there is no
potential deadlock
for this case.
quoted
quoted
FYI, this is what I'd do instead of this patch for now. We don't really
need a vetor per sector, just per page. So this limits the I/O
size a little less.
+
if (max_sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, max_sectors,
GFP_NOIO, &conf->bio_split);
Here the limit is max single-page vectors, and the above way may not work,
such as:ust splitted and not
0 ~ 254: each bvec's length is 512
255: bvec's length is 8192
the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().
Thanks for deeper looking! I guess it is because how vcnt is calculated.
quoted
One solution is to add queue limit of max_single_page_bvec, and let
blk_queue_split() handle it.
The path (blk_queue_split -> blk_bio_segment_split -> bvec_split_segs) which
respects max_segments
of limit. Do you mean introduce max_single_page_bvec to limit? Then perform
similar checking as for
max_segment.
Yes, then the bio is guaranteed to not reach max single-page bvec limit,
just like what __blk_queue_bounce() does.
thanks,
Ming
Yeah, max_sectors is in size units, I messed that up.
quoted
+
if (max_sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, max_sectors,
GFP_NOIO, &conf->bio_split);
Here the limit is max single-page vectors, and the above way may not work,
such as:
0 ~ 254: each bvec's length is 512
255: bvec's length is 8192
the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().
Yes, we still need the rounding magic that alloc_behind_master_bio uses
here.
One solution is to add queue limit of max_single_page_bvec, and let
blk_queue_split() handle it.
Yeah, max_sectors is in size units, I messed that up.
quoted
quoted
+
if (max_sectors < bio_sectors(bio)) {
struct bio *split = bio_split(bio, max_sectors,
GFP_NOIO, &conf->bio_split);
Here the limit is max single-page vectors, and the above way may not work,
such as:
0 ~ 254: each bvec's length is 512
255: bvec's length is 8192
the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().
Yes, we still need the rounding magic that alloc_behind_master_bio uses
here.
But it is wrong to use max sectors to limit number of bvecs(segments), isn't it?
Thanks,
Ming
From: Christoph Hellwig <hch@infradead.org> Date: 2021-08-17 05:07:26
On Mon, Aug 16, 2021 at 07:40:48PM +0800, Ming Lei wrote:
quoted
quoted
0 ~ 254: each bvec's length is 512
255: bvec's length is 8192
the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().
Yes, we still need the rounding magic that alloc_behind_master_bio uses
here.
But it is wrong to use max sectors to limit number of bvecs(segments), isn't it?
The raid1 write behind code cares about the size ofa bio it can reach by
adding order 0 pages to it. The bvecs are part of that and I think the
calculation in the patch documents that a well.
On Tue, Aug 17, 2021 at 06:06:12AM +0100, Christoph Hellwig wrote:
On Mon, Aug 16, 2021 at 07:40:48PM +0800, Ming Lei wrote:
quoted
quoted
quoted
0 ~ 254: each bvec's length is 512
255: bvec's length is 8192
the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().
Yes, we still need the rounding magic that alloc_behind_master_bio uses
here.
But it is wrong to use max sectors to limit number of bvecs(segments), isn't it?
The raid1 write behind code cares about the size ofa bio it can reach by
adding order 0 pages to it. The bvecs are part of that and I think the
calculation in the patch documents that a well.
Thinking of further, your and Guoqing's patch are correct & enough since
bio_copy_data() just copies bytes(sectors) stream from fs bio to the
write behind bio.
Thanks,
Ming
Hi all,
I just had the occasion to test the new patch as landed in arch linux
5.14.7. Unfortunately it does not work for me. Attached you can find a
modification that works for me, though I am not really sure why
write_behind seems not to be set to true on my configuration. If there
is any more data I can provide to help you to investigate, please let me
know.
Thanks for any clues,
Jens
My configuration:
[root@vdr jens]# mdadm --detail -v /dev/md0
/dev/md0:
Version : 1.2
Creation Time : Fri Dec 26 09:50:53 2014
Raid Level : raid1
Array Size : 1953381440 (1862.89 GiB 2000.26 GB)
Used Dev Size : 1953381440 (1862.89 GiB 2000.26 GB)
Raid Devices : 2
Total Devices : 2
Persistence : Superblock is persistent
Intent Bitmap : Internal
Update Time : Fri Sep 24 17:30:51 2021
State : active
Active Devices : 2
Working Devices : 2
Failed Devices : 0
Spare Devices : 0
Consistency Policy : bitmap
Name : vdr:0 (local to host vdr)
UUID : 5532ffda:ccbc790f:b50c4959:8f0fd43f
Events : 32805
Number Major Minor RaidDevice State
2 8 33 0 active sync /dev/sdc1
3 8 17 1 active sync /dev/sdb1
[root@vdr jens]# mdadm -X /dev/sdb1
Filename : /dev/sdb1
Magic : 6d746962
Version : 4
UUID : 5532ffda:ccbc790f:b50c4959:8f0fd43f
Events : 32804
Events Cleared : 32804
State : OK
Chunksize : 64 MB
Daemon : 5s flush period
Write Mode : Allow write behind, max 4096
Sync Size : 1953381440 (1862.89 GiB 2000.26 GB)
Bitmap : 29807 bits (chunks), 3 dirty (0.0%)
[root@vdr jens]# mdadm -X /dev/sdc1
Filename : /dev/sdc1
Magic : 6d746962
Version : 4
UUID : 5532ffda:ccbc790f:b50c4959:8f0fd43f
Events : 32804
Events Cleared : 32804
State : OK
Chunksize : 64 MB
Daemon : 5s flush period
Write Mode : Allow write behind, max 4096
Sync Size : 1953381440 (1862.89 GiB 2000.26 GB)
Bitmap : 29807 bits (chunks), 3 dirty (0.0%)
Am 17.08.21 um 14:32 schrieb Ming Lei:
On Tue, Aug 17, 2021 at 06:06:12AM +0100, Christoph Hellwig wrote:
quoted
On Mon, Aug 16, 2021 at 07:40:48PM +0800, Ming Lei wrote:
quoted
quoted
quoted
0 ~ 254: each bvec's length is 512
255: bvec's length is 8192
the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().
Yes, we still need the rounding magic that alloc_behind_master_bio uses
here.
But it is wrong to use max sectors to limit number of bvecs(segments), isn't it?
The raid1 write behind code cares about the size ofa bio it can reach by
adding order 0 pages to it. The bvecs are part of that and I think the
calculation in the patch documents that a well.
Thinking of further, your and Guoqing's patch are correct & enough since
bio_copy_data() just copies bytes(sectors) stream from fs bio to the
write behind bio.
Thanks,
Ming
Hi all,
I just had the occasion to test the new patch as landed in arch linux
5.14.7. Unfortunately it does not work for me. Attached you can find a
modification that works for me, though I am not really sure why
write_behind seems not to be set to true on my configuration. If there
is any more data I can provide to help you to investigate, please let
me know.
Thanks for the report! As commented in bugzilla, this is because
write-behind
IO still happens even without write-mostly device. I will send new patch
after
you confirm it works.
1. https://bugzilla.kernel.org/show_bug.cgi?id=213181
Thanks,
Guoqing