* [PATCH 1/6] scsi_debug: call map_region() and unmap_region() only when needed
2013-04-16 13:11 [PATCH 0/6] scsi_debug: fix logical block provisioning support Akinobu Mita
@ 2013-04-16 13:11 ` Akinobu Mita
2013-04-25 1:59 ` Martin K. Petersen
2013-04-16 13:11 ` [PATCH 2/6] scsi_debug: prohibit scsi_debug_unmap_granularity == scsi_debug_unmap_alignment Akinobu Mita
` (5 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Akinobu Mita @ 2013-04-16 13:11 UTC (permalink / raw)
To: linux-scsi
Cc: Akinobu Mita, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
If the logical block provisioning is not enabled, map_region() and
unmap_region() have no effect and they don't need to be called.
So this makes map_region() and unmap_region() to be called only
when scsi_debug_lbp() returns true, i.e. logical block provisioning is
enabled.
While I'm at it, this also removes meaningless non-zero check for
scsi_debug_unmap_granularity.
Because scsi_debug_unmap_granularity cannot be zero with usual setting:
scsi_debug_unmap_granularity is 1 by default, and it can be changed to
zero with explicit module parameter setting only when the logical block
provisioning is disabled. But it is only meaningful module parameter
when the logical block provisioning is enabled.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/scsi_debug.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 5cda11c..05abf4e 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2089,7 +2089,7 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
write_lock_irqsave(&atomic_rw, iflags);
ret = do_device_access(SCpnt, devip, lba, num, 1);
- if (scsi_debug_unmap_granularity)
+ if (scsi_debug_lbp())
map_region(lba, num);
write_unlock_irqrestore(&atomic_rw, iflags);
if (-1 == ret)
@@ -2122,7 +2122,7 @@ static int resp_write_same(struct scsi_cmnd *scmd, unsigned long long lba,
write_lock_irqsave(&atomic_rw, iflags);
- if (unmap && scsi_debug_unmap_granularity) {
+ if (unmap && scsi_debug_lbp()) {
unmap_region(lba, num);
goto out;
}
@@ -2146,7 +2146,7 @@ static int resp_write_same(struct scsi_cmnd *scmd, unsigned long long lba,
fake_storep + (lba * scsi_debug_sector_size),
scsi_debug_sector_size);
- if (scsi_debug_unmap_granularity)
+ if (scsi_debug_lbp())
map_region(lba, num);
out:
write_unlock_irqrestore(&atomic_rw, iflags);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/6] scsi_debug: call map_region() and unmap_region() only when needed
2013-04-16 13:11 ` [PATCH 1/6] scsi_debug: call map_region() and unmap_region() only when needed Akinobu Mita
@ 2013-04-25 1:59 ` Martin K. Petersen
0 siblings, 0 replies; 15+ messages in thread
From: Martin K. Petersen @ 2013-04-25 1:59 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-scsi, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
Akinobu> If the logical block provisioning is not enabled, map_region()
Akinobu> and unmap_region() have no effect and they don't need to be
Akinobu> called.
Akinobu> So this makes map_region() and unmap_region() to be called only
Akinobu> when scsi_debug_lbp() returns true, i.e. logical block
Akinobu> provisioning is enabled.
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
Akinobu> Because scsi_debug_unmap_granularity cannot be zero with usual
Akinobu> setting: scsi_debug_unmap_granularity is 1 by default, and it
Akinobu> can be changed to zero with explicit module parameter setting
Akinobu> only when the logical block provisioning is disabled. But it
Akinobu> is only meaningful module parameter when the logical block
Akinobu> provisioning is enabled.
As you have found out, I generally allow module parameter values and
combinations thereof that do not make strict sense. That's because the
main reason for adding these parameters in the first place is to test
the block and SCSI layer code that queries them. scsi_debug is a test
vehicle and being able to simulate a device with broken reporting is
something I use a lot.
That said, I don't think we have any test scripts that actually depend
on this combination of unmap parameters so I'm ok with you cleaning them
up.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/6] scsi_debug: prohibit scsi_debug_unmap_granularity == scsi_debug_unmap_alignment
2013-04-16 13:11 [PATCH 0/6] scsi_debug: fix logical block provisioning support Akinobu Mita
2013-04-16 13:11 ` [PATCH 1/6] scsi_debug: call map_region() and unmap_region() only when needed Akinobu Mita
@ 2013-04-16 13:11 ` Akinobu Mita
2013-04-25 1:37 ` Martin K. Petersen
2013-04-16 13:11 ` [PATCH 3/6] scsi_debug: clear correct memory region when LBPRZ is enabled Akinobu Mita
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Akinobu Mita @ 2013-04-16 13:11 UTC (permalink / raw)
To: linux-scsi
Cc: Akinobu Mita, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
scsi_debug prohibits setting scsi_debug_unmap_alignment to be greater
than scsi_debug_unmap_granularity. But setting them to be the same value
is not prohibited. In this case, the only difference with
scsi_debug_unmap_alignment == 0 is the logical blocks from 0 to
scsi_debug_unmap_alignment - 1 cannot be unmapped. But the difference is
not properly handled in the current code.
So this prohibits such unusual setting.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/scsi_debug.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 05abf4e..5c32140 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3413,9 +3413,10 @@ static int __init scsi_debug_init(void)
clamp(scsi_debug_unmap_granularity, 1U, 0xffffffffU);
if (scsi_debug_unmap_alignment &&
- scsi_debug_unmap_granularity < scsi_debug_unmap_alignment) {
+ scsi_debug_unmap_granularity <=
+ scsi_debug_unmap_alignment) {
printk(KERN_ERR
- "%s: ERR: unmap_granularity < unmap_alignment\n",
+ "%s: ERR: unmap_granularity <= unmap_alignment\n",
__func__);
return -EINVAL;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/6] scsi_debug: prohibit scsi_debug_unmap_granularity == scsi_debug_unmap_alignment
2013-04-16 13:11 ` [PATCH 2/6] scsi_debug: prohibit scsi_debug_unmap_granularity == scsi_debug_unmap_alignment Akinobu Mita
@ 2013-04-25 1:37 ` Martin K. Petersen
0 siblings, 0 replies; 15+ messages in thread
From: Martin K. Petersen @ 2013-04-25 1:37 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-scsi, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
Akinobu> scsi_debug prohibits setting scsi_debug_unmap_alignment to be
Akinobu> greater than scsi_debug_unmap_granularity. But setting them to
Akinobu> be the same value is not prohibited. In this case, the only
Akinobu> difference with scsi_debug_unmap_alignment == 0 is the logical
Akinobu> blocks from 0 to scsi_debug_unmap_alignment - 1 cannot be
Akinobu> unmapped. But the difference is not properly handled in the
Akinobu> current code.
Akinobu> So this prohibits such unusual setting.
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/6] scsi_debug: clear correct memory region when LBPRZ is enabled
2013-04-16 13:11 [PATCH 0/6] scsi_debug: fix logical block provisioning support Akinobu Mita
2013-04-16 13:11 ` [PATCH 1/6] scsi_debug: call map_region() and unmap_region() only when needed Akinobu Mita
2013-04-16 13:11 ` [PATCH 2/6] scsi_debug: prohibit scsi_debug_unmap_granularity == scsi_debug_unmap_alignment Akinobu Mita
@ 2013-04-16 13:11 ` Akinobu Mita
2013-04-25 1:42 ` Martin K. Petersen
2013-04-16 13:11 ` [PATCH 4/6] scsi_debug: add translation functions between LBA and index of provisioning map Akinobu Mita
` (3 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Akinobu Mita @ 2013-04-16 13:11 UTC (permalink / raw)
To: linux-scsi
Cc: Akinobu Mita, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
The function unmap_region() clears memory region specified as the logical
block address and the number of logical blocks in ramdisk storage
(fake_storep) if lbpu and lbprz module parameters are enabled.
In the while loop of unmap_region(), it advances optimal unmap granularity
in logical blocks. But it only clears one logical block at LBA 'block' per
loop iteration. And furthermore, the 'block' is not pointing to a logical
block address which should be cleared, it is a index of probisioning map
(map_storep).
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/scsi_debug.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 5c32140..4b5d388 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2059,8 +2059,9 @@ static void unmap_region(sector_t lba, unsigned int len)
clear_bit(block, map_storep);
if (scsi_debug_lbprz)
memset(fake_storep +
- block * scsi_debug_sector_size, 0,
- scsi_debug_sector_size);
+ lba * scsi_debug_sector_size, 0,
+ scsi_debug_sector_size *
+ scsi_debug_unmap_granularity);
}
lba += granularity - rem;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/6] scsi_debug: clear correct memory region when LBPRZ is enabled
2013-04-16 13:11 ` [PATCH 3/6] scsi_debug: clear correct memory region when LBPRZ is enabled Akinobu Mita
@ 2013-04-25 1:42 ` Martin K. Petersen
0 siblings, 0 replies; 15+ messages in thread
From: Martin K. Petersen @ 2013-04-25 1:42 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-scsi, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
Akinobu> The function unmap_region() clears memory region specified as
Akinobu> the logical block address and the number of logical blocks in
Akinobu> ramdisk storage (fake_storep) if lbpu and lbprz module
Akinobu> parameters are enabled.
Akinobu> In the while loop of unmap_region(), it advances optimal unmap
Akinobu> granularity in logical blocks. But it only clears one logical
Akinobu> block at LBA 'block' per loop iteration. And furthermore, the
Akinobu> 'block' is not pointing to a logical block address which should
Akinobu> be cleared, it is a index of probisioning map (map_storep).
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/6] scsi_debug: add translation functions between LBA and index of provisioning map
2013-04-16 13:11 [PATCH 0/6] scsi_debug: fix logical block provisioning support Akinobu Mita
` (2 preceding siblings ...)
2013-04-16 13:11 ` [PATCH 3/6] scsi_debug: clear correct memory region when LBPRZ is enabled Akinobu Mita
@ 2013-04-16 13:11 ` Akinobu Mita
2013-04-25 1:43 ` Martin K. Petersen
2013-05-02 22:50 ` James Bottomley
2013-04-16 13:11 ` [PATCH 5/6] scsi_debug: fix initialization " Akinobu Mita
` (2 subsequent siblings)
6 siblings, 2 replies; 15+ messages in thread
From: Akinobu Mita @ 2013-04-16 13:11 UTC (permalink / raw)
To: linux-scsi
Cc: Akinobu Mita, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
The translation from LBA to index of provisioning map (map_storep) is used
in various places (map_state(), map_region(), and unmap_region()). But it
is not correctly calculated if scsi_debug_unmap_alignment is zero.
This introduces correct translation functions between LBA and index
of provisioning map:
static unsigned long lba_to_map_index(sector_t lba);
static sector_t map_index_to_lba(unsigned long index);
Actual bug fixes with using these functions will be done by forthcoming
patches.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/scsi_debug.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 4b5d388..c6de36c 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1997,6 +1997,23 @@ out:
return ret;
}
+static unsigned long lba_to_map_index(sector_t lba)
+{
+ if (scsi_debug_unmap_alignment) {
+ lba += scsi_debug_unmap_granularity -
+ scsi_debug_unmap_alignment;
+ }
+ do_div(lba, scsi_debug_unmap_granularity);
+
+ return lba;
+}
+
+static sector_t map_index_to_lba(unsigned long index)
+{
+ return index * scsi_debug_unmap_granularity -
+ scsi_debug_unmap_alignment;
+}
+
static unsigned int map_state(sector_t lba, unsigned int *num)
{
unsigned int granularity, alignment, mapped;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/6] scsi_debug: add translation functions between LBA and index of provisioning map
2013-04-16 13:11 ` [PATCH 4/6] scsi_debug: add translation functions between LBA and index of provisioning map Akinobu Mita
@ 2013-04-25 1:43 ` Martin K. Petersen
2013-05-02 22:50 ` James Bottomley
1 sibling, 0 replies; 15+ messages in thread
From: Martin K. Petersen @ 2013-04-25 1:43 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-scsi, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
Akinobu> The translation from LBA to index of provisioning map
Akinobu> (map_storep) is used in various places (map_state(),
Akinobu> map_region(), and unmap_region()). But it is not correctly
Akinobu> calculated if scsi_debug_unmap_alignment is zero.
Akinobu> This introduces correct translation functions between LBA and
Akinobu> index of provisioning map:
Akinobu> static unsigned long lba_to_map_index(sector_t lba);
Akinobu> static sector_t map_index_to_lba(unsigned long index);
Akinobu> Actual bug fixes with using these functions will be done by
Akinobu> forthcoming patches.
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/6] scsi_debug: add translation functions between LBA and index of provisioning map
2013-04-16 13:11 ` [PATCH 4/6] scsi_debug: add translation functions between LBA and index of provisioning map Akinobu Mita
2013-04-25 1:43 ` Martin K. Petersen
@ 2013-05-02 22:50 ` James Bottomley
1 sibling, 0 replies; 15+ messages in thread
From: James Bottomley @ 2013-05-02 22:50 UTC (permalink / raw)
To: Akinobu Mita; +Cc: linux-scsi, Douglas Gilbert, Martin K. Petersen
On Tue, 2013-04-16 at 22:11 +0900, Akinobu Mita wrote:
> The translation from LBA to index of provisioning map (map_storep) is used
> in various places (map_state(), map_region(), and unmap_region()). But it
> is not correctly calculated if scsi_debug_unmap_alignment is zero.
>
> This introduces correct translation functions between LBA and index
> of provisioning map:
>
> static unsigned long lba_to_map_index(sector_t lba);
> static sector_t map_index_to_lba(unsigned long index);
>
> Actual bug fixes with using these functions will be done by forthcoming
> patches.
That's not the correct way to split patches, because it leads to untidy
things like this:
drivers/scsi/scsi_debug.c:2000:22: warning: ‘lba_to_map_index’ defined
but not used [-Wunused-function]
drivers/scsi/scsi_debug.c:2011:17: warning: ‘map_index_to_lba’ defined
but not used [-Wunused-function]
I fixed this just by rolling the last three patches together, but in
future, just put the static function in with whatever is calling it.
This is also good commit history practise regardless of the static
warning because it adds the function and the callers of that function in
the same commit meaning people who look later don't have to rummage
around for both commits.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/6] scsi_debug: fix initialization of provisioning map
2013-04-16 13:11 [PATCH 0/6] scsi_debug: fix logical block provisioning support Akinobu Mita
` (3 preceding siblings ...)
2013-04-16 13:11 ` [PATCH 4/6] scsi_debug: add translation functions between LBA and index of provisioning map Akinobu Mita
@ 2013-04-16 13:11 ` Akinobu Mita
2013-04-25 1:44 ` Martin K. Petersen
2013-04-16 13:12 ` [PATCH 6/6] scsi_debug: fix logical block provisioning support Akinobu Mita
2013-04-24 23:39 ` [PATCH 0/6] " Douglas Gilbert
6 siblings, 1 reply; 15+ messages in thread
From: Akinobu Mita @ 2013-04-16 13:11 UTC (permalink / raw)
To: linux-scsi
Cc: Akinobu Mita, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
provisioning map (map_storep) is a bitmap accessed by bitops.
So the allocation size should be a multiple of sizeof(unsigned long) and
also the bitmap should be cleared by using bitmap_clear() instead of
memset().
Otherwise it will cause problem on big-endian architecture if the number of
bits is not a multiple of BITS_PER_LONG.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/scsi_debug.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index c6de36c..6581549 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3419,8 +3419,6 @@ static int __init scsi_debug_init(void)
/* Logical Block Provisioning */
if (scsi_debug_lbp()) {
- unsigned int map_bytes;
-
scsi_debug_unmap_max_blocks =
clamp(scsi_debug_unmap_max_blocks, 0U, 0xffffffffU);
@@ -3439,9 +3437,8 @@ static int __init scsi_debug_init(void)
return -EINVAL;
}
- map_size = (sdebug_store_sectors / scsi_debug_unmap_granularity);
- map_bytes = map_size >> 3;
- map_storep = vmalloc(map_bytes);
+ map_size = lba_to_map_index(sdebug_store_sectors - 1) + 1;
+ map_storep = vmalloc(BITS_TO_LONGS(map_size) * sizeof(long));
printk(KERN_INFO "scsi_debug_init: %lu provisioning blocks\n",
map_size);
@@ -3452,7 +3449,7 @@ static int __init scsi_debug_init(void)
goto free_vm;
}
- memset(map_storep, 0x0, map_bytes);
+ bitmap_zero(map_storep, map_size);
/* Map first 1KB for partition table */
if (scsi_debug_num_parts)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 5/6] scsi_debug: fix initialization of provisioning map
2013-04-16 13:11 ` [PATCH 5/6] scsi_debug: fix initialization " Akinobu Mita
@ 2013-04-25 1:44 ` Martin K. Petersen
0 siblings, 0 replies; 15+ messages in thread
From: Martin K. Petersen @ 2013-04-25 1:44 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-scsi, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
Akinobu> provisioning map (map_storep) is a bitmap accessed by bitops.
Akinobu> So the allocation size should be a multiple of sizeof(unsigned
Akinobu> long) and also the bitmap should be cleared by using
Akinobu> bitmap_clear() instead of memset().
Akinobu> Otherwise it will cause problem on big-endian architecture if
Akinobu> the number of bits is not a multiple of BITS_PER_LONG.
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 6/6] scsi_debug: fix logical block provisioning support
2013-04-16 13:11 [PATCH 0/6] scsi_debug: fix logical block provisioning support Akinobu Mita
` (4 preceding siblings ...)
2013-04-16 13:11 ` [PATCH 5/6] scsi_debug: fix initialization " Akinobu Mita
@ 2013-04-16 13:12 ` Akinobu Mita
2013-04-25 1:46 ` Martin K. Petersen
2013-04-24 23:39 ` [PATCH 0/6] " Douglas Gilbert
6 siblings, 1 reply; 15+ messages in thread
From: Akinobu Mita @ 2013-04-16 13:12 UTC (permalink / raw)
To: linux-scsi
Cc: Akinobu Mita, James E.J. Bottomley, Douglas Gilbert,
Martin K. Petersen
I tried testing the logical block provisioning support in scsi_debug,
but it didn't work as I expected.
For example, load scsi_debug module with UNMAP command supported
and fill the storage with random data.
# modprobe scsi_debug lbpu=1
# dd if=/dev/urandom of=/dev/sdb
Then, try to unmap LBA 0, but Get LBA status reports:
# sg_unmap --lba=0 --num=1 /dev/sdb
# sg_get_lba_status --lba=0 /dev/sdb
descriptor LBA: 0x0000000000000000 blocks: 16384 mapped
This is unexpected result. Because UNMAP command to LBA 0 finished
without any errors, but Get LBA status shows that LBA 0 is still mapped.
This problem is due to the wrong translation between LBA and index of
provisioning map. Fix it by using correct translation functions added
previously.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/scsi_debug.c | 55 ++++++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 34 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6581549..154d987 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2016,22 +2016,20 @@ static sector_t map_index_to_lba(unsigned long index)
static unsigned int map_state(sector_t lba, unsigned int *num)
{
- unsigned int granularity, alignment, mapped;
- sector_t block, next, end;
+ sector_t end;
+ unsigned int mapped;
+ unsigned long index;
+ unsigned long next;
- granularity = scsi_debug_unmap_granularity;
- alignment = granularity - scsi_debug_unmap_alignment;
- block = lba + alignment;
- do_div(block, granularity);
-
- mapped = test_bit(block, map_storep);
+ index = lba_to_map_index(lba);
+ mapped = test_bit(index, map_storep);
if (mapped)
- next = find_next_zero_bit(map_storep, map_size, block);
+ next = find_next_zero_bit(map_storep, map_size, index);
else
- next = find_next_bit(map_storep, map_size, block);
+ next = find_next_bit(map_storep, map_size, index);
- end = next * granularity - scsi_debug_unmap_alignment;
+ end = min_t(sector_t, sdebug_store_sectors, map_index_to_lba(next));
*num = end - lba;
return mapped;
@@ -2039,48 +2037,37 @@ static unsigned int map_state(sector_t lba, unsigned int *num)
static void map_region(sector_t lba, unsigned int len)
{
- unsigned int granularity, alignment;
sector_t end = lba + len;
- granularity = scsi_debug_unmap_granularity;
- alignment = granularity - scsi_debug_unmap_alignment;
-
while (lba < end) {
- sector_t block, rem;
-
- block = lba + alignment;
- rem = do_div(block, granularity);
+ unsigned long index = lba_to_map_index(lba);
- if (block < map_size)
- set_bit(block, map_storep);
+ if (index < map_size)
+ set_bit(index, map_storep);
- lba += granularity - rem;
+ lba = map_index_to_lba(index + 1);
}
}
static void unmap_region(sector_t lba, unsigned int len)
{
- unsigned int granularity, alignment;
sector_t end = lba + len;
- granularity = scsi_debug_unmap_granularity;
- alignment = granularity - scsi_debug_unmap_alignment;
-
while (lba < end) {
- sector_t block, rem;
-
- block = lba + alignment;
- rem = do_div(block, granularity);
+ unsigned long index = lba_to_map_index(lba);
- if (rem == 0 && lba + granularity < end && block < map_size) {
- clear_bit(block, map_storep);
- if (scsi_debug_lbprz)
+ if (lba == map_index_to_lba(index) &&
+ lba + scsi_debug_unmap_granularity <= end &&
+ index < map_size) {
+ clear_bit(index, map_storep);
+ if (scsi_debug_lbprz) {
memset(fake_storep +
lba * scsi_debug_sector_size, 0,
scsi_debug_sector_size *
scsi_debug_unmap_granularity);
+ }
}
- lba += granularity - rem;
+ lba = map_index_to_lba(index + 1);
}
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 0/6] scsi_debug: fix logical block provisioning support
2013-04-16 13:11 [PATCH 0/6] scsi_debug: fix logical block provisioning support Akinobu Mita
` (5 preceding siblings ...)
2013-04-16 13:12 ` [PATCH 6/6] scsi_debug: fix logical block provisioning support Akinobu Mita
@ 2013-04-24 23:39 ` Douglas Gilbert
6 siblings, 0 replies; 15+ messages in thread
From: Douglas Gilbert @ 2013-04-24 23:39 UTC (permalink / raw)
To: Akinobu Mita; +Cc: linux-scsi, James E.J. Bottomley, Martin K. Petersen
On 13-04-16 09:11 AM, Akinobu Mita wrote:
> I tried testing the logical block provisioning support in scsi_debug,
> but it didn't work as I expected.
>
> For example, load scsi_debug module with UNMAP command supported
> and fill the storage with random data.
>
> # modprobe scsi_debug lbpu=1
> # dd if=/dev/urandom of=/dev/sdb
>
> Then, try to unmap LBA 0, but Get LBA status reports:
>
> # sg_unmap --lba=0 --num=1 /dev/sdb
> # sg_get_lba_status --lba=0 /dev/sdb
> descriptor LBA: 0x0000000000000000 blocks: 16384 mapped
>
> This is unexpected result. Because UNMAP command to LBA 0 finished
> without any errors, but Get LBA status shows that LBA 0 is still mapped.
>
> I looked around the logical block provisioning support in scsi_debug,
> and I found several problems there. This patch series tries to fix
> these problems and it is broken into small patches as much as possible
> for ease of review.
>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
>
> Akinobu Mita (6):
> scsi_debug: call map_region() and unmap_region() only when needed
> scsi_debug: prohibit scsi_debug_unmap_granularity ==
> scsi_debug_unmap_alignment
> scsi_debug: clear correct memory region when LBPRZ is enabled
> scsi_debug: add translation functions between LBA and index of
> provisioning map
> scsi_debug: fix initialization of provisioning map
> scsi_debug: fix logical block provisioning support
>
I'd like to see some feedback from Martin Petersen on this
set of patches. For my part, for this patch series (1/6
fo 6/6):
Acked-by: Douglas Gilbert <dgilbert@interlog.com>
^ permalink raw reply [flat|nested] 15+ messages in thread