| Message ID | 20180830172504.12348-1-gedwards@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] target/iblock: split T10 PI SGL across command bios | expand |
On 08/30/2018 12:25 PM, Greg Edwards wrote: > When T10 PI is enabled on a backing device for the iblock backstore, the > PI SGL for the entire command is attached to the first bio only. This > works fine if the command is covered by a single bio, but can result in > ref tag errors in the client for the other bios in a multi-bio command, > e.g. > > [ 47.631236] sda: ref tag error at location 2048 (rcvd 0) > [ 47.637658] sda: ref tag error at location 4096 (rcvd 0) > [ 47.644228] sda: ref tag error at location 6144 (rcvd 0) > > The command will be split into multiple bios if the number of data SG > elements exceeds BIO_MAX_PAGES (see iblock_get_bio()). > > The bios may later be split again in the block layer on the host after > iblock_submit_bios(), depending on the queue limits of the backing > device. The block and SCSI layers will pass through the whole PI SGL > down to the LLDD however that first bio is split up, but the LLDD may > only use the portion that corresponds to the data length (depends on the > LLDD, tested with scsi_debug). > > Split the PI SGL across the bios in the command, so each bio's > bio_integrity_payload contains the protection information for the data > in the bio. Use an sg_mapping_iter to keep track of where we are in PI > SGL, so we know where to start with the next bio. > > Signed-off-by: Greg Edwards <gedwards@ddn.com> > --- > Changes from v2: > * add back min(cmd->t_prot_nents, BIO_MAX_PAGES) for bio_integrity_alloc() > from v1 > > Changes from v1: > * expand commit message > * use an sg_mapping_iter to track where we are in the PI SGL > > > drivers/target/target_core_iblock.c | 54 ++++++++++++++++++++--------- > 1 file changed, 37 insertions(+), 17 deletions(-) > > diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c > index ce1321a5cb7b..db78b1c808e8 100644 > --- a/drivers/target/target_core_iblock.c > +++ b/drivers/target/target_core_iblock.c > @@ -635,14 +635,15 @@ static ssize_t iblock_show_configfs_dev_params(struct se_device *dev, char *b) > } > > static int > -iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio) > +iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio, > + struct sg_mapping_iter *miter) > { > struct se_device *dev = cmd->se_dev; > struct blk_integrity *bi; > struct bio_integrity_payload *bip; > struct iblock_dev *ib_dev = IBLOCK_DEV(dev); > - struct scatterlist *sg; > - int i, rc; > + int rc; > + size_t resid, len; > > bi = bdev_get_integrity(ib_dev->ibd_bd); > if (!bi) { > @@ -650,31 +651,39 @@ iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio) > return -ENODEV; > } > > - bip = bio_integrity_alloc(bio, GFP_NOIO, cmd->t_prot_nents); > + bip = bio_integrity_alloc(bio, GFP_NOIO, > + min_t(unsigned int, cmd->t_prot_nents, BIO_MAX_PAGES)); > if (IS_ERR(bip)) { > pr_err("Unable to allocate bio_integrity_payload\n"); > return PTR_ERR(bip); > } > > - bip->bip_iter.bi_size = (cmd->data_length / dev->dev_attrib.block_size) * > - dev->prot_length; > - bip->bip_iter.bi_sector = bio->bi_iter.bi_sector; > + bip->bip_iter.bi_size = bio_integrity_bytes(bi, bio_sectors(bio)); > + bip_set_seed(bip, bio->bi_iter.bi_sector); > > pr_debug("IBLOCK BIP Size: %u Sector: %llu\n", bip->bip_iter.bi_size, > (unsigned long long)bip->bip_iter.bi_sector); > > - for_each_sg(cmd->t_prot_sg, sg, cmd->t_prot_nents, i) { > + resid = bip->bip_iter.bi_size; > + while (resid > 0 && sg_miter_next(miter)) { > > - rc = bio_integrity_add_page(bio, sg_page(sg), sg->length, > - sg->offset); > - if (rc != sg->length) { > + len = min_t(size_t, miter->length, resid); > + rc = bio_integrity_add_page(bio, miter->page, len, > + offset_in_page(miter->addr)); > + if (rc != len) { > pr_err("bio_integrity_add_page() failed; %d\n", rc); > + sg_miter_stop(miter); > return -ENOMEM; > } > > - pr_debug("Added bio integrity page: %p length: %d offset; %d\n", > - sg_page(sg), sg->length, sg->offset); > + pr_debug("Added bio integrity page: %p length: %lu offset: %lu\n", > + miter->page, len, offset_in_page(miter->addr)); > + > + resid -= len; > + if (len < miter->length) > + miter->consumed -= miter->length - len; > } > + sg_miter_stop(miter); > > return 0; > } > @@ -686,12 +695,13 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, > struct se_device *dev = cmd->se_dev; > sector_t block_lba = target_to_linux_sector(dev, cmd->t_task_lba); > struct iblock_req *ibr; > - struct bio *bio, *bio_start; > + struct bio *bio; > struct bio_list list; > struct scatterlist *sg; > u32 sg_num = sgl_nents; > unsigned bio_cnt; > - int i, op, op_flags = 0; > + int i, rc, op, op_flags = 0; > + struct sg_mapping_iter prot_miter; > > if (data_direction == DMA_TO_DEVICE) { > struct iblock_dev *ib_dev = IBLOCK_DEV(dev); > @@ -726,13 +736,17 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, > if (!bio) > goto fail_free_ibr; > > - bio_start = bio; > bio_list_init(&list); > bio_list_add(&list, bio); > > refcount_set(&ibr->pending, 2); > bio_cnt = 1; > > + if (cmd->prot_type && dev->dev_attrib.pi_prot_type) > + sg_miter_start(&prot_miter, cmd->t_prot_sg, cmd->t_prot_nents, > + op == REQ_OP_READ ? SG_MITER_FROM_SG : > + SG_MITER_TO_SG); > + > for_each_sg(sgl, sg, sgl_nents, i) { > /* > * XXX: if the length the device accepts is shorter than the > @@ -741,6 +755,12 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, > */ > while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset) > != sg->length) { > + if (cmd->prot_type && dev->dev_attrib.pi_prot_type) { > + rc = iblock_alloc_bip(cmd, bio, &prot_miter); > + if (rc) > + goto fail_put_bios; > + } > + > if (bio_cnt >= IBLOCK_MAX_BIO_PER_TASK) { > iblock_submit_bios(&list); > bio_cnt = 0; > @@ -762,7 +782,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, > } > > if (cmd->prot_type && dev->dev_attrib.pi_prot_type) { > - int rc = iblock_alloc_bip(cmd, bio_start); > + rc = iblock_alloc_bip(cmd, bio, &prot_miter); > if (rc) > goto fail_put_bios; > } > Seems ok to me. Reviewed-by: Mike Christie <mchristi@redhat.com>
Hi Greg,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on target/for-next]
[also build test WARNING on v4.19-rc1 next-20180831]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Greg-Edwards/target-iblock-split-T10-PI-SGL-across-command-bios/20180831-204618
base: https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next
config: i386-randconfig-h1-09010405 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers//target/target_core_iblock.c: In function 'iblock_alloc_bip':
drivers//target/target_core_iblock.c:663:2: error: implicit declaration of function 'bio_integrity_bytes' [-Werror=implicit-function-declaration]
bip->bip_iter.bi_size = bio_integrity_bytes(bi, bio_sectors(bio));
^
In file included from include/linux/printk.h:329:0,
from include/linux/kernel.h:14,
from include/linux/list.h:9,
from include/linux/timer.h:5,
from drivers//target/target_core_iblock.c:29:
>> include/linux/dynamic_debug.h:75:16: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' [-Wformat=]
static struct _ddebug __aligned(8) \
^
include/linux/dynamic_debug.h:111:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA_KEY'
DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, 0, 0)
^
include/linux/dynamic_debug.h:125:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA'
DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
^
include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
dynamic_pr_debug(fmt, ##__VA_ARGS__)
^
drivers//target/target_core_iblock.c:681:3: note: in expansion of macro 'pr_debug'
pr_debug("Added bio integrity page: %p length: %lu offset: %lu\n",
^
cc1: some warnings being treated as errors
--
drivers/target/target_core_iblock.c: In function 'iblock_alloc_bip':
drivers/target/target_core_iblock.c:663:2: error: implicit declaration of function 'bio_integrity_bytes' [-Werror=implicit-function-declaration]
bip->bip_iter.bi_size = bio_integrity_bytes(bi, bio_sectors(bio));
^
In file included from include/linux/printk.h:329:0,
from include/linux/kernel.h:14,
from include/linux/list.h:9,
from include/linux/timer.h:5,
from drivers/target/target_core_iblock.c:29:
>> include/linux/dynamic_debug.h:75:16: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' [-Wformat=]
static struct _ddebug __aligned(8) \
^
include/linux/dynamic_debug.h:111:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA_KEY'
DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, 0, 0)
^
include/linux/dynamic_debug.h:125:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA'
DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
^
include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
dynamic_pr_debug(fmt, ##__VA_ARGS__)
^
drivers/target/target_core_iblock.c:681:3: note: in expansion of macro 'pr_debug'
pr_debug("Added bio integrity page: %p length: %lu offset: %lu\n",
^
cc1: some warnings being treated as errors
vim +75 include/linux/dynamic_debug.h
ffa10cb4 Jason Baron 2011-08-11 68
b9075fa9 Joe Perches 2011-10-31 69 extern __printf(3, 4)
906d2015 Joe Perches 2014-09-24 70 void __dynamic_netdev_dbg(struct _ddebug *descriptor,
ffa10cb4 Jason Baron 2011-08-11 71 const struct net_device *dev,
b9075fa9 Joe Perches 2011-10-31 72 const char *fmt, ...);
ffa10cb4 Jason Baron 2011-08-11 73
9049fc74 Jason Baron 2016-08-03 74 #define DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, key, init) \
c0d2af63 Joe Perches 2012-10-18 @75 static struct _ddebug __aligned(8) \
07613b0b Jason Baron 2011-10-04 76 __attribute__((section("__verbose"))) name = { \
07613b0b Jason Baron 2011-10-04 77 .modname = KBUILD_MODNAME, \
07613b0b Jason Baron 2011-10-04 78 .function = __func__, \
07613b0b Jason Baron 2011-10-04 79 .filename = __FILE__, \
07613b0b Jason Baron 2011-10-04 80 .format = (fmt), \
07613b0b Jason Baron 2011-10-04 81 .lineno = __LINE__, \
07613b0b Jason Baron 2011-10-04 82 .flags = _DPRINTK_FLAGS_DEFAULT, \
9049fc74 Jason Baron 2016-08-03 83 dd_key_init(key, init) \
07613b0b Jason Baron 2011-10-04 84 }
07613b0b Jason Baron 2011-10-04 85
:::::: The code at line 75 was first introduced by commit
:::::: c0d2af637863940b1a4fb208224ca7acb905c39f dynamic_debug: Remove unnecessary __used
:::::: TO: Joe Perches <joe@perches.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, Sep 01, 2018 at 08:42:16AM +0800, kbuild test robot wrote: > Hi Greg, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on target/for-next] > [also build test WARNING on v4.19-rc1 next-20180831] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Greg-Edwards/target-iblock-split-T10-PI-SGL-across-command-bios/20180831-204618 > base: https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next > config: i386-randconfig-h1-09010405 (attached as .config) > compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > All warnings (new ones prefixed by >>): > > drivers//target/target_core_iblock.c: In function 'iblock_alloc_bip': > drivers//target/target_core_iblock.c:663:2: error: implicit declaration of function 'bio_integrity_bytes' [-Werror=implicit-function-declaration] > bip->bip_iter.bi_size = bio_integrity_bytes(bi, bio_sectors(bio)); > ^ > In file included from include/linux/printk.h:329:0, > from include/linux/kernel.h:14, > from include/linux/list.h:9, > from include/linux/timer.h:5, > from drivers//target/target_core_iblock.c:29: Should the 0-day being using a different target tree/branch? The for-next branch above hasn't been updated since January. Greg
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c index ce1321a5cb7b..db78b1c808e8 100644 --- a/drivers/target/target_core_iblock.c +++ b/drivers/target/target_core_iblock.c @@ -635,14 +635,15 @@ static ssize_t iblock_show_configfs_dev_params(struct se_device *dev, char *b) } static int -iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio) +iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio, + struct sg_mapping_iter *miter) { struct se_device *dev = cmd->se_dev; struct blk_integrity *bi; struct bio_integrity_payload *bip; struct iblock_dev *ib_dev = IBLOCK_DEV(dev); - struct scatterlist *sg; - int i, rc; + int rc; + size_t resid, len; bi = bdev_get_integrity(ib_dev->ibd_bd); if (!bi) { @@ -650,31 +651,39 @@ iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio) return -ENODEV; } - bip = bio_integrity_alloc(bio, GFP_NOIO, cmd->t_prot_nents); + bip = bio_integrity_alloc(bio, GFP_NOIO, + min_t(unsigned int, cmd->t_prot_nents, BIO_MAX_PAGES)); if (IS_ERR(bip)) { pr_err("Unable to allocate bio_integrity_payload\n"); return PTR_ERR(bip); } - bip->bip_iter.bi_size = (cmd->data_length / dev->dev_attrib.block_size) * - dev->prot_length; - bip->bip_iter.bi_sector = bio->bi_iter.bi_sector; + bip->bip_iter.bi_size = bio_integrity_bytes(bi, bio_sectors(bio)); + bip_set_seed(bip, bio->bi_iter.bi_sector); pr_debug("IBLOCK BIP Size: %u Sector: %llu\n", bip->bip_iter.bi_size, (unsigned long long)bip->bip_iter.bi_sector); - for_each_sg(cmd->t_prot_sg, sg, cmd->t_prot_nents, i) { + resid = bip->bip_iter.bi_size; + while (resid > 0 && sg_miter_next(miter)) { - rc = bio_integrity_add_page(bio, sg_page(sg), sg->length, - sg->offset); - if (rc != sg->length) { + len = min_t(size_t, miter->length, resid); + rc = bio_integrity_add_page(bio, miter->page, len, + offset_in_page(miter->addr)); + if (rc != len) { pr_err("bio_integrity_add_page() failed; %d\n", rc); + sg_miter_stop(miter); return -ENOMEM; } - pr_debug("Added bio integrity page: %p length: %d offset; %d\n", - sg_page(sg), sg->length, sg->offset); + pr_debug("Added bio integrity page: %p length: %lu offset: %lu\n", + miter->page, len, offset_in_page(miter->addr)); + + resid -= len; + if (len < miter->length) + miter->consumed -= miter->length - len; } + sg_miter_stop(miter); return 0; } @@ -686,12 +695,13 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, struct se_device *dev = cmd->se_dev; sector_t block_lba = target_to_linux_sector(dev, cmd->t_task_lba); struct iblock_req *ibr; - struct bio *bio, *bio_start; + struct bio *bio; struct bio_list list; struct scatterlist *sg; u32 sg_num = sgl_nents; unsigned bio_cnt; - int i, op, op_flags = 0; + int i, rc, op, op_flags = 0; + struct sg_mapping_iter prot_miter; if (data_direction == DMA_TO_DEVICE) { struct iblock_dev *ib_dev = IBLOCK_DEV(dev); @@ -726,13 +736,17 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, if (!bio) goto fail_free_ibr; - bio_start = bio; bio_list_init(&list); bio_list_add(&list, bio); refcount_set(&ibr->pending, 2); bio_cnt = 1; + if (cmd->prot_type && dev->dev_attrib.pi_prot_type) + sg_miter_start(&prot_miter, cmd->t_prot_sg, cmd->t_prot_nents, + op == REQ_OP_READ ? SG_MITER_FROM_SG : + SG_MITER_TO_SG); + for_each_sg(sgl, sg, sgl_nents, i) { /* * XXX: if the length the device accepts is shorter than the @@ -741,6 +755,12 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, */ while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset) != sg->length) { + if (cmd->prot_type && dev->dev_attrib.pi_prot_type) { + rc = iblock_alloc_bip(cmd, bio, &prot_miter); + if (rc) + goto fail_put_bios; + } + if (bio_cnt >= IBLOCK_MAX_BIO_PER_TASK) { iblock_submit_bios(&list); bio_cnt = 0; @@ -762,7 +782,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, } if (cmd->prot_type && dev->dev_attrib.pi_prot_type) { - int rc = iblock_alloc_bip(cmd, bio_start); + rc = iblock_alloc_bip(cmd, bio, &prot_miter); if (rc) goto fail_put_bios; }
When T10 PI is enabled on a backing device for the iblock backstore, the PI SGL for the entire command is attached to the first bio only. This works fine if the command is covered by a single bio, but can result in ref tag errors in the client for the other bios in a multi-bio command, e.g. [ 47.631236] sda: ref tag error at location 2048 (rcvd 0) [ 47.637658] sda: ref tag error at location 4096 (rcvd 0) [ 47.644228] sda: ref tag error at location 6144 (rcvd 0) The command will be split into multiple bios if the number of data SG elements exceeds BIO_MAX_PAGES (see iblock_get_bio()). The bios may later be split again in the block layer on the host after iblock_submit_bios(), depending on the queue limits of the backing device. The block and SCSI layers will pass through the whole PI SGL down to the LLDD however that first bio is split up, but the LLDD may only use the portion that corresponds to the data length (depends on the LLDD, tested with scsi_debug). Split the PI SGL across the bios in the command, so each bio's bio_integrity_payload contains the protection information for the data in the bio. Use an sg_mapping_iter to keep track of where we are in PI SGL, so we know where to start with the next bio. Signed-off-by: Greg Edwards <gedwards@ddn.com> --- Changes from v2: * add back min(cmd->t_prot_nents, BIO_MAX_PAGES) for bio_integrity_alloc() from v1 Changes from v1: * expand commit message * use an sg_mapping_iter to track where we are in the PI SGL drivers/target/target_core_iblock.c | 54 ++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 17 deletions(-)