* [PATCH 2/3] target/file: Fix SG table for prot_buf initialization
2015-04-13 14:21 [PATCH 1/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled Akinobu Mita
@ 2015-04-13 14:21 ` Akinobu Mita
2015-04-16 5:16 ` Nicholas A. Bellinger
2015-04-13 14:21 ` [PATCH 3/3] target/file: Fix UNMAP with DIF protection support Akinobu Mita
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Akinobu Mita @ 2015-04-13 14:21 UTC (permalink / raw)
To: target-devel
Cc: Akinobu Mita, Nicholas Bellinger, Sagi Grimberg,
Martin K. Petersen, Christoph Hellwig, James E.J. Bottomley,
linux-scsi
In fd_do_prot_rw(), it allocates prot_buf which is used to copy from
se_cmd->t_prot_sg by sbc_dif_copy_prot(). The SG table for prot_buf
is also initialized by allocating 'se_cmd->t_prot_nents' entries of
scatterlist and setting the data length of each entry to PAGE_SIZE
at most.
However if se_cmd->t_prot_sg contains a clustered entry (i.e.
sg->length > PAGE_SIZE), the SG table for prot_buf can't be
initialized correctly and sbc_dif_copy_prot() can't copy to prot_buf.
(This actually happened with TCM loopback fabric module)
As prot_buf is allocated by kzalloc() and it's physically contiguous,
we only need a single scatterlist entry.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
drivers/target/target_core_file.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 8ca1883..4c7a6c8 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -264,11 +264,10 @@ static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot,
struct se_device *se_dev = cmd->se_dev;
struct fd_dev *dev = FD_DEV(se_dev);
struct file *prot_fd = dev->fd_prot_file;
- struct scatterlist *sg;
loff_t pos = (cmd->t_task_lba * se_dev->prot_length);
unsigned char *buf;
- u32 prot_size, len, size;
- int rc, ret = 1, i;
+ u32 prot_size;
+ int rc, ret = 1;
prot_size = (cmd->data_length / se_dev->dev_attrib.block_size) *
se_dev->prot_length;
@@ -281,24 +280,16 @@ static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot,
}
buf = fd_prot->prot_buf;
- fd_prot->prot_sg_nents = cmd->t_prot_nents;
- fd_prot->prot_sg = kzalloc(sizeof(struct scatterlist) *
- fd_prot->prot_sg_nents, GFP_KERNEL);
+ fd_prot->prot_sg_nents = 1;
+ fd_prot->prot_sg = kzalloc(sizeof(struct scatterlist),
+ GFP_KERNEL);
if (!fd_prot->prot_sg) {
pr_err("Unable to allocate fd_prot->prot_sg\n");
kfree(fd_prot->prot_buf);
return -ENOMEM;
}
sg_init_table(fd_prot->prot_sg, fd_prot->prot_sg_nents);
- size = prot_size;
-
- for_each_sg(fd_prot->prot_sg, sg, fd_prot->prot_sg_nents, i) {
-
- len = min_t(u32, PAGE_SIZE, size);
- sg_set_buf(sg, buf, len);
- size -= len;
- buf += len;
- }
+ sg_set_buf(fd_prot->prot_sg, buf, prot_size);
}
if (is_write) {
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] target/file: Fix SG table for prot_buf initialization
2015-04-13 14:21 ` [PATCH 2/3] target/file: Fix SG table for prot_buf initialization Akinobu Mita
@ 2015-04-16 5:16 ` Nicholas A. Bellinger
0 siblings, 0 replies; 9+ messages in thread
From: Nicholas A. Bellinger @ 2015-04-16 5:16 UTC (permalink / raw)
To: Akinobu Mita
Cc: target-devel, Sagi Grimberg, Martin K. Petersen,
Christoph Hellwig, James E.J. Bottomley, linux-scsi
On Mon, 2015-04-13 at 23:21 +0900, Akinobu Mita wrote:
> In fd_do_prot_rw(), it allocates prot_buf which is used to copy from
> se_cmd->t_prot_sg by sbc_dif_copy_prot(). The SG table for prot_buf
> is also initialized by allocating 'se_cmd->t_prot_nents' entries of
> scatterlist and setting the data length of each entry to PAGE_SIZE
> at most.
>
> However if se_cmd->t_prot_sg contains a clustered entry (i.e.
> sg->length > PAGE_SIZE), the SG table for prot_buf can't be
> initialized correctly and sbc_dif_copy_prot() can't copy to prot_buf.
> (This actually happened with TCM loopback fabric module)
>
> As prot_buf is allocated by kzalloc() and it's physically contiguous,
> we only need a single scatterlist entry.
>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: target-devel@vger.kernel.org
> Cc: linux-scsi@vger.kernel.org
> ---
> drivers/target/target_core_file.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
Applied to target-pending/for-next, with CC' for v3.14.y stable.
Thanks Akinobu!
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] target/file: Fix UNMAP with DIF protection support
2015-04-13 14:21 [PATCH 1/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled Akinobu Mita
2015-04-13 14:21 ` [PATCH 2/3] target/file: Fix SG table for prot_buf initialization Akinobu Mita
@ 2015-04-13 14:21 ` Akinobu Mita
2015-04-13 15:07 ` Sagi Grimberg
` (2 more replies)
2015-04-13 14:55 ` [PATCH 1/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled Sagi Grimberg
2015-04-16 5:10 ` Nicholas A. Bellinger
3 siblings, 3 replies; 9+ messages in thread
From: Akinobu Mita @ 2015-04-13 14:21 UTC (permalink / raw)
To: target-devel
Cc: Akinobu Mita, Nicholas Bellinger, Sagi Grimberg,
Martin K. Petersen, Christoph Hellwig, James E.J. Bottomley,
linux-scsi
When UNMAP command is issued with DIF protection support enabled,
the protection info for the unmapped region is remain unchanged.
So READ command for the region causes data integrity failure.
This fixes it by invalidating protection info for the unmapped region
by filling with 0xff pattern. This change also adds helper function
fd_do_prot_fill() in order to reduce code duplication with existing
fd_format_prot().
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
drivers/target/target_core_file.c | 86 +++++++++++++++++++++++++++------------
1 file changed, 61 insertions(+), 25 deletions(-)
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 4c7a6c8..cbb0cc2 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -541,6 +541,56 @@ fd_execute_write_same(struct se_cmd *cmd)
return 0;
}
+static int
+fd_do_prot_fill(struct se_device *se_dev, sector_t lba, sector_t nolb,
+ void *buf, size_t bufsize)
+{
+ struct fd_dev *fd_dev = FD_DEV(se_dev);
+ struct file *prot_fd = fd_dev->fd_prot_file;
+ sector_t prot_length, prot;
+ loff_t pos = lba * se_dev->prot_length;
+
+ if (!prot_fd) {
+ pr_err("Unable to locate fd_dev->fd_prot_file\n");
+ return -ENODEV;
+ }
+
+ prot_length = nolb * se_dev->prot_length;
+
+ for (prot = 0; prot < prot_length;) {
+ sector_t len = min_t(sector_t, bufsize, prot_length - prot);
+ ssize_t ret = kernel_write(prot_fd, buf, len, pos + prot);
+
+ if (ret != len) {
+ pr_err("vfs_write to prot file failed: %zd\n", ret);
+ return ret < 0 ? ret : -ENODEV;
+ }
+ prot += ret;
+ }
+
+ return 0;
+}
+
+static int
+fd_do_prot_unmap(struct se_cmd *cmd, sector_t lba, sector_t nolb)
+{
+ void *buf;
+ int rc;
+
+ buf = (void *)__get_free_page(GFP_KERNEL);
+ if (!buf) {
+ pr_err("Unable to allocate FILEIO prot buf\n");
+ return -ENOMEM;
+ }
+ memset(buf, 0xff, PAGE_SIZE);
+
+ rc = fd_do_prot_fill(cmd->se_dev, lba, nolb, buf, PAGE_SIZE);
+
+ free_page((unsigned long)buf);
+
+ return rc;
+}
+
static sense_reason_t
fd_do_unmap(struct se_cmd *cmd, void *priv, sector_t lba, sector_t nolb)
{
@@ -548,6 +598,12 @@ fd_do_unmap(struct se_cmd *cmd, void *priv, sector_t lba, sector_t nolb)
struct inode *inode = file->f_mapping->host;
int ret;
+ if (cmd->se_dev->dev_attrib.pi_prot_type) {
+ ret = fd_do_prot_unmap(cmd, lba, nolb);
+ if (ret)
+ return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+ }
+
if (S_ISBLK(inode->i_mode)) {
/* The backend is block device, use discard */
struct block_device *bdev = inode->i_bdev;
@@ -870,48 +926,28 @@ static int fd_init_prot(struct se_device *dev)
static int fd_format_prot(struct se_device *dev)
{
- struct fd_dev *fd_dev = FD_DEV(dev);
- struct file *prot_fd = fd_dev->fd_prot_file;
- sector_t prot_length, prot;
unsigned char *buf;
- loff_t pos = 0;
int unit_size = FDBD_FORMAT_UNIT_SIZE * dev->dev_attrib.block_size;
- int rc, ret = 0, size, len;
+ int ret;
if (!dev->dev_attrib.pi_prot_type) {
pr_err("Unable to format_prot while pi_prot_type == 0\n");
return -ENODEV;
}
- if (!prot_fd) {
- pr_err("Unable to locate fd_dev->fd_prot_file\n");
- return -ENODEV;
- }
buf = vzalloc(unit_size);
if (!buf) {
pr_err("Unable to allocate FILEIO prot buf\n");
return -ENOMEM;
}
- prot_length = (dev->transport->get_blocks(dev) + 1) * dev->prot_length;
- size = prot_length;
pr_debug("Using FILEIO prot_length: %llu\n",
- (unsigned long long)prot_length);
+ (unsigned long long)(dev->transport->get_blocks(dev) + 1) *
+ dev->prot_length);
memset(buf, 0xff, unit_size);
- for (prot = 0; prot < prot_length; prot += unit_size) {
- len = min(unit_size, size);
- rc = kernel_write(prot_fd, buf, len, pos);
- if (rc != len) {
- pr_err("vfs_write to prot file failed: %d\n", rc);
- ret = -ENODEV;
- goto out;
- }
- pos += len;
- size -= len;
- }
-
-out:
+ ret = fd_do_prot_fill(dev, 0, dev->transport->get_blocks(dev) + 1,
+ buf, unit_size);
vfree(buf);
return ret;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] target/file: Fix UNMAP with DIF protection support
2015-04-13 14:21 ` [PATCH 3/3] target/file: Fix UNMAP with DIF protection support Akinobu Mita
@ 2015-04-13 15:07 ` Sagi Grimberg
2015-04-14 1:20 ` Martin K. Petersen
2015-04-16 5:21 ` Nicholas A. Bellinger
2 siblings, 0 replies; 9+ messages in thread
From: Sagi Grimberg @ 2015-04-13 15:07 UTC (permalink / raw)
To: Akinobu Mita, target-devel
Cc: Nicholas Bellinger, Sagi Grimberg, Martin K. Petersen,
Christoph Hellwig, James E.J. Bottomley, linux-scsi
On 4/13/2015 5:21 PM, Akinobu Mita wrote:
> When UNMAP command is issued with DIF protection support enabled,
> the protection info for the unmapped region is remain unchanged.
> So READ command for the region causes data integrity failure.
>
> This fixes it by invalidating protection info for the unmapped region
> by filling with 0xff pattern. This change also adds helper function
> fd_do_prot_fill() in order to reduce code duplication with existing
> fd_format_prot().
>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: target-devel@vger.kernel.org
> Cc: linux-scsi@vger.kernel.org
> ---
> drivers/target/target_core_file.c | 86 +++++++++++++++++++++++++++------------
> 1 file changed, 61 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 4c7a6c8..cbb0cc2 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -541,6 +541,56 @@ fd_execute_write_same(struct se_cmd *cmd)
> return 0;
> }
>
> +static int
> +fd_do_prot_fill(struct se_device *se_dev, sector_t lba, sector_t nolb,
> + void *buf, size_t bufsize)
> +{
> + struct fd_dev *fd_dev = FD_DEV(se_dev);
> + struct file *prot_fd = fd_dev->fd_prot_file;
> + sector_t prot_length, prot;
> + loff_t pos = lba * se_dev->prot_length;
> +
> + if (!prot_fd) {
> + pr_err("Unable to locate fd_dev->fd_prot_file\n");
> + return -ENODEV;
> + }
> +
> + prot_length = nolb * se_dev->prot_length;
> +
> + for (prot = 0; prot < prot_length;) {
> + sector_t len = min_t(sector_t, bufsize, prot_length - prot);
> + ssize_t ret = kernel_write(prot_fd, buf, len, pos + prot);
> +
> + if (ret != len) {
> + pr_err("vfs_write to prot file failed: %zd\n", ret);
> + return ret < 0 ? ret : -ENODEV;
> + }
> + prot += ret;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +fd_do_prot_unmap(struct se_cmd *cmd, sector_t lba, sector_t nolb)
> +{
> + void *buf;
> + int rc;
> +
> + buf = (void *)__get_free_page(GFP_KERNEL);
> + if (!buf) {
> + pr_err("Unable to allocate FILEIO prot buf\n");
> + return -ENOMEM;
> + }
> + memset(buf, 0xff, PAGE_SIZE);
> +
> + rc = fd_do_prot_fill(cmd->se_dev, lba, nolb, buf, PAGE_SIZE);
> +
> + free_page((unsigned long)buf);
> +
> + return rc;
> +}
> +
> static sense_reason_t
> fd_do_unmap(struct se_cmd *cmd, void *priv, sector_t lba, sector_t nolb)
> {
> @@ -548,6 +598,12 @@ fd_do_unmap(struct se_cmd *cmd, void *priv, sector_t lba, sector_t nolb)
> struct inode *inode = file->f_mapping->host;
> int ret;
>
> + if (cmd->se_dev->dev_attrib.pi_prot_type) {
> + ret = fd_do_prot_unmap(cmd, lba, nolb);
> + if (ret)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + }
> +
> if (S_ISBLK(inode->i_mode)) {
> /* The backend is block device, use discard */
> struct block_device *bdev = inode->i_bdev;
> @@ -870,48 +926,28 @@ static int fd_init_prot(struct se_device *dev)
>
> static int fd_format_prot(struct se_device *dev)
> {
> - struct fd_dev *fd_dev = FD_DEV(dev);
> - struct file *prot_fd = fd_dev->fd_prot_file;
> - sector_t prot_length, prot;
> unsigned char *buf;
> - loff_t pos = 0;
> int unit_size = FDBD_FORMAT_UNIT_SIZE * dev->dev_attrib.block_size;
> - int rc, ret = 0, size, len;
> + int ret;
>
> if (!dev->dev_attrib.pi_prot_type) {
> pr_err("Unable to format_prot while pi_prot_type == 0\n");
> return -ENODEV;
> }
> - if (!prot_fd) {
> - pr_err("Unable to locate fd_dev->fd_prot_file\n");
> - return -ENODEV;
> - }
>
> buf = vzalloc(unit_size);
> if (!buf) {
> pr_err("Unable to allocate FILEIO prot buf\n");
> return -ENOMEM;
> }
> - prot_length = (dev->transport->get_blocks(dev) + 1) * dev->prot_length;
> - size = prot_length;
>
> pr_debug("Using FILEIO prot_length: %llu\n",
> - (unsigned long long)prot_length);
> + (unsigned long long)(dev->transport->get_blocks(dev) + 1) *
> + dev->prot_length);
>
> memset(buf, 0xff, unit_size);
> - for (prot = 0; prot < prot_length; prot += unit_size) {
> - len = min(unit_size, size);
> - rc = kernel_write(prot_fd, buf, len, pos);
> - if (rc != len) {
> - pr_err("vfs_write to prot file failed: %d\n", rc);
> - ret = -ENODEV;
> - goto out;
> - }
> - pos += len;
> - size -= len;
> - }
> -
> -out:
> + ret = fd_do_prot_fill(dev, 0, dev->transport->get_blocks(dev) + 1,
> + buf, unit_size);
> vfree(buf);
> return ret;
> }
>
Thanks for this needed patch.
Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] target/file: Fix UNMAP with DIF protection support
2015-04-13 14:21 ` [PATCH 3/3] target/file: Fix UNMAP with DIF protection support Akinobu Mita
2015-04-13 15:07 ` Sagi Grimberg
@ 2015-04-14 1:20 ` Martin K. Petersen
2015-04-16 5:21 ` Nicholas A. Bellinger
2 siblings, 0 replies; 9+ messages in thread
From: Martin K. Petersen @ 2015-04-14 1:20 UTC (permalink / raw)
To: Akinobu Mita
Cc: target-devel, Nicholas Bellinger, Sagi Grimberg,
Martin K. Petersen, Christoph Hellwig, James E.J. Bottomley,
linux-scsi
>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
Akinobu> When UNMAP command is issued with DIF protection support
Akinobu> enabled, the protection info for the unmapped region is remain
Akinobu> unchanged. So READ command for the region causes data
Akinobu> integrity failure.
Akinobu> This fixes it by invalidating protection info for the unmapped
Akinobu> region by filling with 0xff pattern. This change also adds
Akinobu> helper function fd_do_prot_fill() in order to reduce code
Akinobu> duplication with existing fd_format_prot().
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] target/file: Fix UNMAP with DIF protection support
2015-04-13 14:21 ` [PATCH 3/3] target/file: Fix UNMAP with DIF protection support Akinobu Mita
2015-04-13 15:07 ` Sagi Grimberg
2015-04-14 1:20 ` Martin K. Petersen
@ 2015-04-16 5:21 ` Nicholas A. Bellinger
2 siblings, 0 replies; 9+ messages in thread
From: Nicholas A. Bellinger @ 2015-04-16 5:21 UTC (permalink / raw)
To: Akinobu Mita
Cc: target-devel, Sagi Grimberg, Martin K. Petersen,
Christoph Hellwig, James E.J. Bottomley, linux-scsi
On Mon, 2015-04-13 at 23:21 +0900, Akinobu Mita wrote:
> When UNMAP command is issued with DIF protection support enabled,
> the protection info for the unmapped region is remain unchanged.
> So READ command for the region causes data integrity failure.
>
> This fixes it by invalidating protection info for the unmapped region
> by filling with 0xff pattern. This change also adds helper function
> fd_do_prot_fill() in order to reduce code duplication with existing
> fd_format_prot().
>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: target-devel@vger.kernel.org
> Cc: linux-scsi@vger.kernel.org
> ---
> drivers/target/target_core_file.c | 86 +++++++++++++++++++++++++++------------
> 1 file changed, 61 insertions(+), 25 deletions(-)
>
Applied to for-next with CC' for v3.14.y stable.
Nice work on this series, and thanks for including detailed changelog
messages.
Thanks Akinobu!
--nab
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled
2015-04-13 14:21 [PATCH 1/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled Akinobu Mita
2015-04-13 14:21 ` [PATCH 2/3] target/file: Fix SG table for prot_buf initialization Akinobu Mita
2015-04-13 14:21 ` [PATCH 3/3] target/file: Fix UNMAP with DIF protection support Akinobu Mita
@ 2015-04-13 14:55 ` Sagi Grimberg
2015-04-16 5:10 ` Nicholas A. Bellinger
3 siblings, 0 replies; 9+ messages in thread
From: Sagi Grimberg @ 2015-04-13 14:55 UTC (permalink / raw)
To: Akinobu Mita, target-devel
Cc: Nicholas Bellinger, Sagi Grimberg, Martin K. Petersen,
Christoph Hellwig, James E.J. Bottomley, linux-scsi
On 4/13/2015 5:21 PM, Akinobu Mita wrote:
> When CONFIG_DEBUG_SG=y and DIF protection support enabled, kernel
> BUG()s are triggered due to the following two issues:
>
> 1) prot_sg is not initialized by sg_init_table().
>
> When CONFIG_DEBUG_SG=y, scatterlist helpers check sg entry has a
> correct magic value.
>
> 2) vmalloc'ed buffer is passed to sg_set_buf().
>
> sg_set_buf() uses virt_to_page() to convert virtual address to struct
> page, but it doesn't work with vmalloc address. vmalloc_to_page()
> should be used instead. As prot_buf isn't usually too large, so
> fix it by allocating prot_buf by kmalloc instead of vmalloc.
>
Hi Akinobu,
This obviously fixes a bug, but I'm afraid that for certain
workloads (large IO size) this would trigger higher order allocations.
how about removing the prot_buf altogether? The only reason why
this bounce is used is to allow code sharing with rd_mcp DIF mode
calling sbc_verify_[write|read]. But I'd say it doesn't make sense
anymore given these fixes.
IMO, the t_prot_sg can be used just like t_data_sg.
In the read case, we just read the data into t_prot_sg using
bvec_iter (better to reuse fd_do_rw code) and just call
__sbc_dif_verify_read (that would not copy sg's).
In the write case, we call sbc_dif_verify_write() with NULL to
avoid the copy and then just write it to the file (reusing fd_do_rw
code).
Thoughts?
Sagi.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled
2015-04-13 14:21 [PATCH 1/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled Akinobu Mita
` (2 preceding siblings ...)
2015-04-13 14:55 ` [PATCH 1/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled Sagi Grimberg
@ 2015-04-16 5:10 ` Nicholas A. Bellinger
3 siblings, 0 replies; 9+ messages in thread
From: Nicholas A. Bellinger @ 2015-04-16 5:10 UTC (permalink / raw)
To: Akinobu Mita
Cc: target-devel, Sagi Grimberg, Martin K. Petersen,
Christoph Hellwig, James E.J. Bottomley, linux-scsi
On Mon, 2015-04-13 at 23:21 +0900, Akinobu Mita wrote:
> When CONFIG_DEBUG_SG=y and DIF protection support enabled, kernel
> BUG()s are triggered due to the following two issues:
>
> 1) prot_sg is not initialized by sg_init_table().
>
> When CONFIG_DEBUG_SG=y, scatterlist helpers check sg entry has a
> correct magic value.
>
> 2) vmalloc'ed buffer is passed to sg_set_buf().
>
> sg_set_buf() uses virt_to_page() to convert virtual address to struct
> page, but it doesn't work with vmalloc address. vmalloc_to_page()
> should be used instead. As prot_buf isn't usually too large, so
> fix it by allocating prot_buf by kmalloc instead of vmalloc.
>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: target-devel@vger.kernel.org
> Cc: linux-scsi@vger.kernel.org
> ---
> drivers/target/target_core_file.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
Thanks for testing with CONFIG_DEBUG_SG=y.
Applied to target-pending/for-next, with CC' for v3.14.y stable.
Thank you,
--nab
^ permalink raw reply [flat|nested] 9+ messages in thread