qemu-img: conditionally discard target on convert

Signed-off-by Peter Lieven <[email protected]>

---
 block/raw.c |    6 ++++++
 1 file changed, 6 insertions(+)

block/raw.c +5 -0

index 8c81de9..f1682d4 100644
@@ -121,6 +121,11 @@ static int raw_has_zero_init(BlockDriverState *bs)
     return bdrv_has_zero_init(bs->file);
 }
 
+static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    return bdrv_get_info(bs->file, bdi);
+}
+
 static BlockDriver bdrv_raw = {
     .format_name        = "raw",
 

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Signed-off-by Peter Lieven <[email protected]>

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

Il 17/07/2013 10:46, Kevin Wolf ha scritto:

Quoted 5 lines

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 8 lines

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

I thought the same yesterday when reviewing the series, but I'm not convinced.

Discarding is not always the right way to write zeroes, because it can disrupt performance. It may be fine when you are already going to write a sparse image (as is the case for qemu-img convert), but not in general. So if you just used write_zeroes, it would have to fall under yet another -drive option (or an extension to "-drive discard"). I think what Peter did is a good compromise in the end.

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Am 17.07.2013 um 11:58 schrieb Paolo Bonzini <[email protected]>:

Quoted 25 lines

Il 17/07/2013 10:46, Kevin Wolf ha scritto:

Quoted 5 lines

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 8 lines

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

I thought the same yesterday when reviewing the series, but I'm not convinced.

Discarding is not always the right way to write zeroes, because it can disrupt performance. It may be fine when you are already going to write a sparse image (as is the case for qemu-img convert), but not in general. So if you just used write_zeroes, it would have to fall under yet another -drive option (or an extension to "-drive discard"). I think what Peter did is a good compromise in the end.

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Good question, I will pass it to ronnie. My guess is that the command will fail with a check condition if it failed to unmap the data. From what Ronnie sent earlier it should be guaranteed that the blocks are at least zero after the unmap command.

As for the qemu-img patch this shouldn't matter. It uses always blocks of bdi->max_unmap which should be a multiple of the alignment. It also checks if sectors are deallocated after the unmap afterwards. If the unmap fails it falls back to has_zero_init =1.

Am 17.07.2013 um 12:21 hat Peter Lieven geschrieben:

Quoted 35 lines

Am 17.07.2013 um 11:58 schrieb Paolo Bonzini <[email protected]>:

Quoted 25 lines

Il 17/07/2013 10:46, Kevin Wolf ha scritto:

Quoted 5 lines

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 8 lines

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

I thought the same yesterday when reviewing the series, but I'm not convinced.

Discarding is not always the right way to write zeroes, because it can disrupt performance. It may be fine when you are already going to write a sparse image (as is the case for qemu-img convert), but not in general. So if you just used write_zeroes, it would have to fall under yet another -drive option (or an extension to "-drive discard"). I think what Peter did is a good compromise in the end.

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Good question, I will pass it to ronnie. My guess is that the command will fail with a check condition if it failed to unmap the data. From what Ronnie sent earlier it should be guaranteed that the blocks are at least zero after the unmap command.

Quoted 3 lines

As for the qemu-img patch this shouldn't matter. It uses always blocks of bdi->max_unmap which should be a multiple of the alignment. It also checks if sectors are deallocated after the unmap afterwards. If the unmap fails it falls back to has_zero_init =1.

Well, you use bdrv_discard(), and ignoring discards is valid. Just another reason to use bdrv_write_zeroes() instead.

Am 17.07.2013 um 12:21 hat Peter Lieven geschrieben:

Quoted 35 lines

Am 17.07.2013 um 11:58 schrieb Paolo Bonzini <[email protected]>:

Quoted 25 lines

Il 17/07/2013 10:46, Kevin Wolf ha scritto:

Quoted 5 lines

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 8 lines

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

I thought the same yesterday when reviewing the series, but I'm not convinced.

Discarding is not always the right way to write zeroes, because it can disrupt performance. It may be fine when you are already going to write a sparse image (as is the case for qemu-img convert), but not in general. So if you just used write_zeroes, it would have to fall under yet another -drive option (or an extension to "-drive discard"). I think what Peter did is a good compromise in the end.

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Good question, I will pass it to ronnie. My guess is that the command will fail with a check condition if it failed to unmap the data. From what Ronnie sent earlier it should be guaranteed that the blocks are at least zero after the unmap command.

Quoted 3 lines

As for the qemu-img patch this shouldn't matter. It uses always blocks of bdi->max_unmap which should be a multiple of the alignment. It also checks if sectors are deallocated after the unmap afterwards. If the unmap fails it falls back to has_zero_init =1.

Well, you use bdrv_discard(), and ignoring discards is valid. Just another reason to use bdrv_write_zeroes() instead.

Il 17/07/2013 12:27, Kevin Wolf ha scritto:

Quoted 40 lines

Am 17.07.2013 um 12:21 hat Peter Lieven geschrieben:

Quoted 38 lines

Am 17.07.2013 um 11:58 schrieb Paolo Bonzini <[email protected]>:

Quoted 25 lines

Il 17/07/2013 10:46, Kevin Wolf ha scritto:

Quoted 5 lines

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 8 lines

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

I thought the same yesterday when reviewing the series, but I'm not convinced.

Discarding is not always the right way to write zeroes, because it can disrupt performance. It may be fine when you are already going to write a sparse image (as is the case for qemu-img convert), but not in general. So if you just used write_zeroes, it would have to fall under yet another -drive option (or an extension to "-drive discard"). I think what Peter did is a good compromise in the end.

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Good question, I will pass it to ronnie. My guess is that the command will fail with a check condition if it failed to unmap the data. From what Ronnie sent earlier it should be guaranteed that the blocks are at least zero after the unmap command.

As for the qemu-img patch this shouldn't matter. It uses always blocks of bdi->max_unmap which should be a multiple of the alignment. It also checks if sectors are deallocated after the unmap afterwards. If the unmap fails it falls back to has_zero_init =1.

Quoted 2 lines

Well, you use bdrv_discard(), and ignoring discards is valid. Just another reason to use bdrv_write_zeroes() instead.

He's only using it if discard_zeroes is true in the new BlockDriverInfo. We can define the semantics of that bit, and I think defining it as "ignored discards will still write zeroes" is a good thing (same as what SCSI targets do if you use WRITE SAME to do the discard operation).

Paolo

Quoted 2 lines

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Good question, I will pass it to ronnie. My guess is that the command will fail with a check condition if it failed to unmap the data. From what Ronnie sent earlier it should be guaranteed that the blocks are at least zero after the unmap command.

As for the qemu-img patch this shouldn't matter. It uses always blocks of bdi->max_unmap which should be a multiple of the alignment.

Il 17/07/2013 12:21, Peter Lieven ha scritto:

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Good question, I will pass it to ronnie. My guess is that the command will fail with a check condition if it failed to unmap the data. From what Ronnie sent earlier it should be guaranteed that the blocks are at least zero after the unmap command.

I'm not so sure about that, SBC says explicitly that a misaligned unmap request "may result in unmap operations on fewer LBAs than requested".

Perhaps it's better to do this:

- if LBPWS=1, use WRITE SAME(16). For WRITE SAME it is explicit that "if the device server does not deallocate or anchor the LBA, then the device server shall perform the specified write operation". Expose the value of LBPRZ in the "discard zeroes" field of BlockDriverInfo.

- if LBPWS=0, you can still support discard iff LBPU=1 and use UNMAP. But if LBPRZ=1 and LBPWS=0, you should not expose the "discard zeroes" field of BlockDriverInfo.

Quoted 2 lines

As for the qemu-img patch this shouldn't matter. It uses always blocks of bdi->max_unmap which should be a multiple of the alignment.

Also, the remainder itself (which SBC calls "UNMAP GRANULARITY ALIGNMENT") may not be 0. I think the above trick (only exposing LBPRZ if you'll use WRITE SAME) will sidestep the problem. It may also remove the need to fall back to has_zero_init=1.

Paolo

Am 17.07.2013 um 12:45 schrieb Paolo Bonzini <[email protected]>:

Quoted 24 lines

Il 17/07/2013 12:21, Peter Lieven ha scritto:

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Good question, I will pass it to ronnie. My guess is that the command will fail with a check condition if it failed to unmap the data. From what Ronnie sent earlier it should be guaranteed that the blocks are at least zero after the unmap command.

I'm not so sure about that, SBC says explicitly that a misaligned unmap request "may result in unmap operations on fewer LBAs than requested".

Perhaps it's better to do this:

- if LBPWS=1, use WRITE SAME(16). For WRITE SAME it is explicit that "if the device server does not deallocate or anchor the LBA, then the device server shall perform the specified write operation". Expose the value of LBPRZ in the "discard zeroes" field of BlockDriverInfo.

- if LBPWS=0, you can still support discard iff LBPU=1 and use UNMAP. But if LBPRZ=1 and LBPWS=0, you should not expose the "discard zeroes" field of BlockDriverInfo.

Quoted 2 lines

As for the qemu-img patch this shouldn't matter. It uses always blocks of bdi->max_unmap which should be a multiple of the alignment.

Quoted 4 lines

Also, the remainder itself (which SBC calls "UNMAP GRANULARITY ALIGNMENT") may not be 0. I think the above trick (only exposing LBPRZ if you'll use WRITE SAME) will sidestep the problem. It may also remove the need to fall back to has_zero_init=1.

You mean has_zero_init = 0?

What is if the max write same size and max unmap differ?

Il 17/07/2013 10:46, Kevin Wolf ha scritto:

Quoted 5 lines

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 8 lines

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

I thought the same yesterday when reviewing the series, but I'm not convinced.

Discarding is not always the right way to write zeroes, because it can disrupt performance. It may be fine when you are already going to write a sparse image (as is the case for qemu-img convert), but not in general. So if you just used write_zeroes, it would have to fall under yet another -drive option (or an extension to "-drive discard").

I think what Peter did is a good compromise in the end.

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

Am 17.07.2013 um 11:58 hat Paolo Bonzini geschrieben:

Quoted 15 lines

Il 17/07/2013 10:46, Kevin Wolf ha scritto:

Quoted 5 lines

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 8 lines

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

Quoted 8 lines

I thought the same yesterday when reviewing the series, but I'm not convinced.

Discarding is not always the right way to write zeroes, because it can disrupt performance. It may be fine when you are already going to write a sparse image (as is the case for qemu-img convert), but not in general. So if you just used write_zeroes, it would have to fall under yet another -drive option (or an extension to "-drive discard").

Maybe we need a flag to bdrv_write_zeroes() that specifies whether to use discard or not, kind of like the unmap bit in WRITE SAME.

On the other hand, I think you're right that this is really policy, and as such shouldn't be hardcoded based on our guesses, but be configurable.

Quoted 1 line

I think what Peter did is a good compromise in the end.

At the very least it must become a separate function. img_convert() is already too big and too deeply nested.

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

SBC-3 Rev. 35e, 5.16 READ CAPACITY (16) command:

"A logical block provisioning read zeros (LBPRZ) bit set to one indicates that, for read commands specifying an unmapped LBA (see 4.7.4.5), the device server returns user data set to zero [...]"

So it depends on the block provisioning state of the LBA, not on the operations that were performed on it.

5.28 UNMAP command:

If the ANCHOR bit in the CDB is set to zero, and the logical unit is thin provisioned (see 4.7.3.3), then the logical block provisioning state for each specified LBA:

a) should become deallocated;
b) may become anchored; or
c) may remain unchanged.

So with UNMAP, I think you don't have any guarantees that the LBA becomes unmapped and therefore zeroed. It could just keep its current data. No matter whether your request was aligned or not.

I think it is reasonable to assume that IF LBPRZ==1 and IF it is an "optimal unmap request" then the blocks will become unmapped and they will read back as 0.

Il 17/07/2013 17:54, ronnie sahlberg ha scritto:

Quoted 3 lines

I think it is reasonable to assume that IF LBPRZ==1 and IF it is an "optimal unmap request" then the blocks will become unmapped and they will read back as 0.

Yes, but it is not reasonable to assume that bdrv_discard will only receive "optimal" requests. Thus, using WRITE SAME for LBPRZ=1, and not exposing LBPRZ=1 if LBPWS=0 (may also use LBPWS10 depending on the capacity), is the safer thing to do.

Paolo

Quoted 11 lines

Il 17/07/2013 17:54, ronnie sahlberg ha scritto:

Quoted 3 lines

I think it is reasonable to assume that IF LBPRZ==1 and IF it is an "optimal unmap request" then the blocks will become unmapped and they will read back as 0.

Yes, but it is not reasonable to assume that bdrv_discard will only receive "optimal" requests. Thus, using WRITE SAME for LBPRZ=1, and not exposing LBPRZ=1 if LBPWS=0 (may also use LBPWS10 depending on the capacity), is the safer thing to do.

Paolo

ACK.

WRITESAME10/16 with UNMAP flag set is probably best.

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Von meinem iPhone gesendet

Am 17.07.2013 um 18:31 schrieb ronnie sahlberg <[email protected]>:

Quoted 16 lines · Paolo Bonzini

Quoted 11 lines

Il 17/07/2013 17:54, ronnie sahlberg ha scritto:

Quoted 3 lines

I think it is reasonable to assume that IF LBPRZ==1 and IF it is an "optimal unmap request" then the blocks will become unmapped and they will read back as 0.

Yes, but it is not reasonable to assume that bdrv_discard will only receive "optimal" requests. Thus, using WRITE SAME for LBPRZ=1, and not exposing LBPRZ=1 if LBPWS=0 (may also use LBPWS10 depending on the capacity), is the safer thing to do.

Paolo

ACK.

WRITESAME10/16 with UNMAP flag set is probably best.

Il 17/07/2013 19:02, Peter Lieven ha scritto:

Quoted 1 line

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Yes.

Remember you can still use UNMAP if LBPRZ=0.

Paolo

Quoted 20 lines

Von meinem iPhone gesendet

Am 17.07.2013 um 18:31 schrieb ronnie sahlberg <[email protected]>:

Quoted 16 lines · Paolo Bonzini

Quoted 11 lines

Il 17/07/2013 17:54, ronnie sahlberg ha scritto:

Quoted 3 lines

I think it is reasonable to assume that IF LBPRZ==1 and IF it is an "optimal unmap request" then the blocks will become unmapped and they will read back as 0.

Yes, but it is not reasonable to assume that bdrv_discard will only receive "optimal" requests. Thus, using WRITE SAME for LBPRZ=1, and not exposing LBPRZ=1 if LBPWS=0 (may also use LBPWS10 depending on the capacity), is the safer thing to do.

Paolo

ACK.

WRITESAME10/16 with UNMAP flag set is probably best.

Am 17.07.2013 um 19:04 schrieb Paolo Bonzini <[email protected]>:

Quoted 5 lines

Il 17/07/2013 19:02, Peter Lieven ha scritto:

Quoted 1 line

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Yes.

Quoted 1 line

Remember you can still use UNMAP if LBPRZ=0.

I can always use it if writesame is not available, but in this case bdi->discard_zeroes must be 0.

Maybe we should call it discard_writes_zeroes or similar.

Discard_zeroes is sth that should only indicate if lbprz == 1. At least if we refer to the Linux ioctl. We could include both in BDI.

Am 17.07.2013 um 19:04 schrieb Paolo Bonzini <[email protected]>:

Quoted 5 lines

Il 17/07/2013 19:02, Peter Lieven ha scritto:

Quoted 1 line

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Yes.

Quoted 1 line

Remember you can still use UNMAP if LBPRZ=0.

I can always use it if writesame is not available, but in this case bdi->discard_zeroes must be 0.

Maybe we should call it discard_writes_zeroes or similar.

Discard_zeroes is sth that should only indicate if lbprz == 1. At least if we refer to the Linux ioctl. We could include both in BDI.

Am 17.07.2013 um 19:48 hat Peter Lieven geschrieben:

Quoted 12 lines

Am 17.07.2013 um 19:04 schrieb Paolo Bonzini <[email protected]>:

Quoted 6 lines

Il 17/07/2013 19:02, Peter Lieven ha scritto:

Quoted 1 line

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Yes.

Remember you can still use UNMAP if LBPRZ=0.

I can always use it if writesame is not available, but in this case bdi->discard_zeroes must be 0.

Quoted 3 lines

Maybe we should call it discard_writes_zeroes or similar.

Discard_zeroes is sth that should only indicate if lbprz == 1. At least if we refer to the Linux ioctl. We could include both in BDI.

Maybe what we really should do is to define different operations (with an exact behaviour) instead of having one bdrv_discard() and then adding flags everywhere to tell what the operation is doing exactly.

Il 18/07/2013 11:23, Kevin Wolf ha scritto:

Quoted 15 lines

Am 17.07.2013 um 19:48 hat Peter Lieven geschrieben:

Quoted 14 lines

Am 17.07.2013 um 19:04 schrieb Paolo Bonzini <[email protected]>:

Quoted 6 lines

Il 17/07/2013 19:02, Peter Lieven ha scritto:

Quoted 1 line

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Yes.

Remember you can still use UNMAP if LBPRZ=0.

I can always use it if writesame is not available, but in this case bdi->discard_zeroes must be 0.

Maybe we should call it discard_writes_zeroes or similar.

Quoted 5 lines

Quoted 1 line

Discard_zeroes is sth that should only indicate if lbprz == 1. At least if we refer to the Linux ioctl. We could include both in BDI.

Maybe what we really should do is to define different operations (with an exact behaviour) instead of having one bdrv_discard() and then adding flags everywhere to tell what the operation is doing exactly.

A BDRV_MAY_UNMAP flag for bdrv_write_zeroes?

Il 18/07/2013 11:23, Kevin Wolf ha scritto:

Quoted 10 lines

Am 17.07.2013 um 19:48 hat Peter Lieven geschrieben:

Quoted 9 lines

Am 17.07.2013 um 19:04 schrieb Paolo Bonzini <[email protected]>:

Quoted 5 lines

Il 17/07/2013 19:02, Peter Lieven ha scritto:

Quoted 1 line

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Yes.

Remember you can still use UNMAP if LBPRZ=0.

I can always use it if writesame is not available, but in this case bdi->discard_zeroes must be 0.

Quoted 6 lines

Quoted 3 lines

Maybe we should call it discard_writes_zeroes or similar.

Discard_zeroes is sth that should only indicate if lbprz == 1. At least if we refer to the Linux ioctl. We could include both in BDI.

Maybe what we really should do is to define different operations (with an exact behaviour) instead of having one bdrv_discard() and then adding flags everywhere to tell what the operation is doing exactly.

A BDRV_MAY_UNMAP flag for bdrv_write_zeroes?

Quoted 17 lines

Il 18/07/2013 11:23, Kevin Wolf ha scritto:

Quoted 16 lines

Am 17.07.2013 um 19:48 hat Peter Lieven geschrieben:

Quoted 15 lines

Am 17.07.2013 um 19:04 schrieb Paolo Bonzini <[email protected]>:

Quoted 6 lines

Il 17/07/2013 19:02, Peter Lieven ha scritto:

Quoted 2 lines

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Yes.

Remember you can still use UNMAP if LBPRZ=0.

I can always use it if writesame is not available, but in this case bdi->discard_zeroes must be 0.

Maybe we should call it discard_writes_zeroes or similar.

Discard_zeroes is sth that should only indicate if lbprz == 1. At least if we refer to the Linux ioctl. We could include both in BDI.

Quoted 4 lines

Quoted 3 lines

Maybe what we really should do is to define different operations (with an exact behaviour) instead of having one bdrv_discard() and then adding flags everywhere to tell what the operation is doing exactly.

A BDRV_MAY_UNMAP flag for bdrv_write_zeroes?

I thought that we wanted to add a paramter to the BDI (call it write_zeroes_w_discard).

If this is set the bdrv MUST accept a flag to bdrv_discard() lets call it BDRV_DISCARD_WRITE_ZEROES
and he has to ensure that all sectors specified in bdrv_discard() read as zero after the operation.

If this flag is not set (e.g. when the OS issues a normal discard) the operation may still silently fail with
undefined provisioning state and content of the specified sectors.

With the above flag a general optimization of bdrv_write_zeroes would be possible, that calls bdrv_discard with BDRV_DISCARD_WRITE_ZEROES if bdi->write_zeroes_w_discard == 1.

The second flag that could be added is bdi->discard_zeroes. But this only says unmapped blocks read back as zero.

Il 18/07/2013 12:44, Peter Lieven ha scritto:

Quoted 16 lines · Paolo Bonzini

Quoted 15 lines

Il 18/07/2013 11:23, Kevin Wolf ha scritto:

Quoted 14 lines

Am 17.07.2013 um 19:48 hat Peter Lieven geschrieben:

Quoted 13 lines

Am 17.07.2013 um 19:04 schrieb Paolo Bonzini <[email protected]>:

Quoted 6 lines

Il 17/07/2013 19:02, Peter Lieven ha scritto:

Quoted 2 lines

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Yes.

Remember you can still use UNMAP if LBPRZ=0.

I can always use it if writesame is not available, but in this case bdi->discard_zeroes must be 0.

Maybe we should call it discard_writes_zeroes or similar.

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Discard_zeroes is sth that should only indicate if lbprz == 1. At least if we refer to the Linux ioctl. We could include both in BDI.

Maybe what we really should do is to define different operations (with an exact behaviour) instead of having one bdrv_discard() and then adding flags everywhere to tell what the operation is doing exactly.

A BDRV_MAY_UNMAP flag for bdrv_write_zeroes?

I thought that we wanted to add a paramter to the BDI (call it write_zeroes_w_discard).

I also thought so, but I like Kevin's idea of not shoehorning it in bdrv_discard(). Extending bdrv_write_zeroes is a better API.

So far we've avoided "discard zeroes" semantics in QEMU (device models are also careful not to expose that). Since the only sane way to implement what you want is to use the SCSI WRITE SAME command, adding flags to bdrv_write_zeroes will even be easier because the mapping with SCSI is natural: discard = UNMAP, write_zeroes = WRITE SAME (either without or with the UNMAP bit).

Quoted 5 lines

If this is set the bdrv MUST accept a flag to bdrv_discard() lets call it BDRV_DISCARD_WRITE_ZEROES
and he has to ensure that all sectors specified in bdrv_discard() read as zero after the operation.

Quoted 3 lines

If this flag is not set (e.g. when the OS issues a normal discard) the operation may still silently fail with
undefined provisioning state and content of the specified sectors.

But if you set BDRV_DISCARD_WRITE_ZEROES, then you always need a fallback to bdrv_write_zeroes. Why not just call bdrv_write_zeroes to begin with? That's why extending bdrv_write_zeroes is preferable.

Quoted 3 lines

But if you set BDRV_DISCARD_WRITE_ZEROES, then you always need a fallback to bdrv_write_zeroes. Why not just call bdrv_write_zeroes to begin with? That's why extending bdrv_write_zeroes is preferable.

In this case wo do not need a flag to the function at all. If the driver sets bdi->write_zeroes_w_discard = 1 then bdrv_write_zeroes can use bdrv_discard to write zeroes and the driver has to ensure that all is zero afterwards.

If the driver would have a better method of writing zeroes than discard it simply should not set bdi->write_zeroes_w_discard = 1.

Il 18/07/2013 13:04, Peter Lieven ha scritto:

Quoted 7 lines

Quoted 3 lines

But if you set BDRV_DISCARD_WRITE_ZEROES, then you always need a fallback to bdrv_write_zeroes. Why not just call bdrv_write_zeroes to begin with? That's why extending bdrv_write_zeroes is preferable.

In this case wo do not need a flag to the function at all. If the driver sets bdi->write_zeroes_w_discard = 1 then bdrv_write_zeroes can use bdrv_discard to write zeroes and the driver has to ensure that all is zero afterwards.

Peter, you removed exactly the part of the email where I explained the wrong part of your reasoning:

you cannot do that [discard in bdrv_write_zeroes] unconditionally. Some operations can use it, some cannot. Think of SCSI disk
emulation: it must not discard is WRITE SAME is sent without the UNMAP bit!

Quoted 2 lines

If the driver would have a better method of writing zeroes than discard it simply should not set bdi->write_zeroes_w_discard = 1.

If the driver had a better method of writing zeroes than discard, it simply should ignore the BDRV_MAY_UNMAP (or BDRV_MAY_DISCARD) flag in its bdrv_write_zeros implementation.

Quoted 5 lines

Quoted 2 lines

If the driver would have a better method of writing zeroes than discard it simply should not set bdi->write_zeroes_w_discard = 1.

If the driver had a better method of writing zeroes than discard, it simply should ignore the BDRV_MAY_UNMAP (or BDRV_MAY_DISCARD) flag in its bdrv_write_zeros implementation.

ok, but this would require an individual patch in every driver, wouldn't it. i am ok with that.

the BDRV_MAY_DISCARD flag is at the end a hint if the driver can optimize writing zeroes by a discard or if real zeroes should be written e.g. to sanitize the device?

talking for iscsi:
bdrv->discard can remain to use UNMAP and silently fail if lbpu == 0. bdrv->write_zeroes will use writesame16 and set the unmap flag only if BDRV_MAY_DISCARD == 1 and BDRV_O_UNMAP == 1 and lbprz == 1. in case lbpws == 0 it will return -ENOSUP.

Correct?

Peter

Il 18/07/2013 15:29, Peter Lieven ha scritto:

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

If the driver would have a better method of writing zeroes than discard it simply should not set bdi->write_zeroes_w_discard = 1.

If the driver had a better method of writing zeroes than discard, it simply should ignore the BDRV_MAY_UNMAP (or BDRV_MAY_DISCARD) flag in its bdrv_write_zeros implementation.

ok, but this would require an individual patch in every driver, wouldn't it. i am ok with that.

Yes (making the drivers return the flag in the BDI would also require per-driver patches).

BDRV_MAY_UNMAP is an advisory flag, that the driver is free to ignore (just like bdrv_discard can be ignored altogether).

block.c probably should avoid passing down BDRV_MAY_UNMAP altogether if the BDRV_O_UNMAP flag is not set.

Quoted 4 lines

the BDRV_MAY_DISCARD flag is at the end a hint if the driver can optimize writing zeroes by a discard or if real zeroes should be written e.g. to sanitize the device?

Quoted 5 lines

talking for iscsi:
bdrv->discard can remain to use UNMAP and silently fail if lbpu == 0. bdrv->write_zeroes will use writesame16 and set the unmap flag only if BDRV_MAY_DISCARD == 1 and BDRV_O_UNMAP == 1 and lbprz == 1. in case lbpws == 0 it will return -ENOSUP.

Almost: for LBPWS == 0 it can use WRITE SAME(16) without the unmap flag.
If that doesn't work either it can return -ENOTSUP.

Quoted 10 lines

Il 18/07/2013 15:29, Peter Lieven ha scritto:

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

If the driver would have a better method of writing zeroes than discard it simply should not set bdi->write_zeroes_w_discard = 1.

If the driver had a better method of writing zeroes than discard, it simply should ignore the BDRV_MAY_UNMAP (or BDRV_MAY_DISCARD) flag in its bdrv_write_zeros implementation.

ok, but this would require an individual patch in every driver, wouldn't it. i am ok with that.

Yes (making the drivers return the flag in the BDI would also require per-driver patches).

we still might need a hint for qemu-img convert that the driver does support
writing zeroes by unmap because using write_zeroes in the main loop might result in unaligned requests that the target is not able to unmap. and to avoid writing several blocks twice by first writing all zeroes to the target and then writing all data blocks again I would need to keep the loop
at the beginning of qemu-img convert to write zeroes with correct alignment and granularity if the driver supports write_zeroes_w_discard.

Il 18/07/2013 16:09, Peter Lieven ha scritto:

Quoted 11 lines · Paolo Bonzini

Quoted 10 lines

Il 18/07/2013 15:29, Peter Lieven ha scritto:

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

If the driver would have a better method of writing zeroes than discard it simply should not set bdi->write_zeroes_w_discard = 1.

If the driver had a better method of writing zeroes than discard, it simply should ignore the BDRV_MAY_UNMAP (or BDRV_MAY_DISCARD) flag in its bdrv_write_zeros implementation.

ok, but this would require an individual patch in every driver, wouldn't it. i am ok with that.

Yes (making the drivers return the flag in the BDI would also require per-driver patches).

Quoted 9 lines

we still might need a hint for qemu-img convert that the driver does support
writing zeroes by unmap because using write_zeroes in the main loop might result in unaligned requests that the target is not able to unmap. and to avoid writing several blocks twice by first writing all zeroes to the target and then writing all data blocks again I would need to keep the loop
at the beginning of qemu-img convert to write zeroes with correct alignment and granularity if the driver supports write_zeroes_w_discard.

(Mis)alignment and granularity can be handled later. We can ignore them for now. Later, if we decide the best way to support them is a flag, we'll add it. Let's not put the cart before the horse.

BTW, I expect alignment!=0 to be really, really rare.

Quoted 5 lines

Quoted 1 line

(Mis)alignment and granularity can be handled later. We can ignore them for now. Later, if we decide the best way to support them is a flag, we'll add it. Let's not put the cart before the horse.

Quoted 1 line

BTW, I expect alignment!=0 to be really, really rare.

To explain my concerns:

I know that my target has internal page size of 15MB. I will check what happens
if I deallocate this 15MB in chunks of lets say 1MB. If the page gets unprovisioned
after the last chunk is unmapped it would be fine :-)

Il 18/07/2013 16:32, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

(Mis)alignment and granularity can be handled later. We can ignore them for now. Later, if we decide the best way to support them is a flag, we'll add it. Let's not put the cart before the horse.

BTW, I expect alignment!=0 to be really, really rare.

To explain my concerns:

Quoted 5 lines

I know that my target has internal page size of 15MB. I will check what happens
if I deallocate this 15MB in chunks of lets say 1MB. If the page gets unprovisioned
after the last chunk is unmapped it would be fine :-)

You're talking of granularity here, not (mis)alignment.

Am 18.07.2013 um 16:35 schrieb Paolo Bonzini <[email protected]>:

Quoted 9 lines

Il 18/07/2013 16:32, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

(Mis)alignment and granularity can be handled later. We can ignore them for now. Later, if we decide the best way to support them is a flag, we'll add it. Let's not put the cart before the horse.

BTW, I expect alignment!=0 to be really, really rare.

To explain my concerns:

Quoted 7 lines

Quoted 5 lines

I know that my target has internal page size of 15MB. I will check what happens
if I deallocate this 15MB in chunks of lets say 1MB. If the page gets unprovisioned
after the last chunk is unmapped it would be fine :-)

You're talking of granularity here, not (mis)alignment.

you are right. for the target i am talking about this is 30720 512-byte blocks for the granularity (pagesize) and 0 for the alignment.
i will see what happens if I write same w/unmap the whole 30720 blocks in smaller blocks ;-) otherwise i will have to add support for honoring this values in qemu-img convert as a follow up.

Peter

BlockLimitsVPD OptimalUnmapGranularity also applies to unmapping with writesame16 :

An OPTIMAL UNMAP GRANULARITY field set to a non-zero value indicates the optimal granularity in logical blocks
for unmap requests (e.g., an UNMAP command or a WRITE SAME (16) command with the UNMAP bit set to
one). An unmap request with a number of logical blocks that is not a multiple of this value may result in unmap
operations on fewer LBAs than requested. An OPTIMAL UNMAP GRANULARITY field set to 0000_0000h indicates
that the device server does not report an optimal unmap granularity.

So if you send a writesame16+unmap-bit that honours the "optimal unmap request" you have a pretty good chance
that the blocks will be unmapped. If they are not they will at least be overwritten as zero.

Quoted 26 lines

Am 18.07.2013 um 16:35 schrieb Paolo Bonzini <[email protected]>:

Quoted 9 lines

Il 18/07/2013 16:32, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

(Mis)alignment and granularity can be handled later. We can ignore them for now. Later, if we decide the best way to support them is a flag, we'll add it. Let's not put the cart before the horse.

BTW, I expect alignment!=0 to be really, really rare.

To explain my concerns:

Quoted 7 lines

Quoted 5 lines

I know that my target has internal page size of 15MB. I will check what happens
if I deallocate this 15MB in chunks of lets say 1MB. If the page gets unprovisioned
after the last chunk is unmapped it would be fine :-)

You're talking of granularity here, not (mis)alignment.

you are right. for the target i am talking about this is 30720 512-byte blocks for the granularity (pagesize) and 0 for the alignment.
i will see what happens if I write same w/unmap the whole 30720 blocks in smaller blocks ;-) otherwise i will have to add support for honoring this values in qemu-img convert as a follow up.

Peter

thanks for the details. I think to have optimal performance and best change for unmapping in qemu-img convert
it might be best to export the OPTIMAL UNMAP GRANULARITY

as well as the write_zeroes_w_discard capability via the BDI

and than zero
out the whole device with bdrv_write_zeroes and the BDRV_MAY_DISCARD flag.

the logic for this is already prepared in patch4 (topic of this email). except that i have to exchange bdrv_discard with bdrv_write_zeroes w/ BDRV_MAY_DISCARD.

what do you think?

Quoted 29 lines · Peter Lieven

Quoted 26 lines

Am 18.07.2013 um 16:35 schrieb Paolo Bonzini <[email protected]>:

Quoted 9 lines

Il 18/07/2013 16:32, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

(Mis)alignment and granularity can be handled later. We can ignore them for now. Later, if we decide the best way to support them is a flag, we'll add it. Let's not put the cart before the horse.

BTW, I expect alignment!=0 to be really, really rare.

To explain my concerns:

Quoted 7 lines

Quoted 5 lines

I know that my target has internal page size of 15MB. I will check what happens
if I deallocate this 15MB in chunks of lets say 1MB. If the page gets unprovisioned
after the last chunk is unmapped it would be fine :-)

You're talking of granularity here, not (mis)alignment.

you are right. for the target i am talking about this is 30720 512-byte blocks for the granularity (pagesize) and 0 for the alignment.
i will see what happens if I write same w/unmap the whole 30720 blocks in smaller blocks ;-) otherwise i will have to add support for honoring this values in qemu-img convert as a follow up.

Peter

Il 18/07/2013 21:28, Peter Lieven ha scritto:

Quoted 3 lines

thanks for the details. I think to have optimal performance and best change for unmapping in qemu-img convert
it might be best to export the OPTIMAL UNMAP GRANULARITY

Agreed about this.

Quoted 1 line

as well as the write_zeroes_w_discard capability via the BDI

But why this?!? It is _not_ needed. All you need is to change the default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is nonzero.

Paolo

Quoted 37 lines

and than zero
out the whole device with bdrv_write_zeroes and the BDRV_MAY_DISCARD flag.

the logic for this is already prepared in patch4 (topic of this email). except that i have to exchange bdrv_discard with bdrv_write_zeroes w/ BDRV_MAY_DISCARD.

what do you think?

Quoted 24 lines · Peter Lieven

Quoted 22 lines

Am 18.07.2013 um 16:35 schrieb Paolo Bonzini <[email protected]>:

Quoted 6 lines

Il 18/07/2013 16:32, Peter Lieven ha scritto:

Quoted 5 lines

Quoted 5 lines

(Mis)alignment and granularity can be handled later. We can ignore them for now. Later, if we decide the best way to support them is a flag, we'll add it. Let's not put the cart before the horse.

BTW, I expect alignment!=0 to be really, really rare.

Quoted 8 lines

Quoted 7 lines

To explain my concerns:

I know that my target has internal page size of 15MB. I will check what happens
if I deallocate this 15MB in chunks of lets say 1MB. If the page gets unprovisioned
after the last chunk is unmapped it would be fine :-)

You're talking of granularity here, not (mis)alignment.

you are right. for the target i am talking about this is 30720 512-byte blocks for the granularity (pagesize) and 0 for the alignment.
i will see what happens if I write same w/unmap the whole 30720 blocks in smaller blocks ;-) otherwise i will have to add support for honoring this values in qemu-img convert as a follow up.

Peter

Quoted 7 lines · Paolo Bonzini

Il 18/07/2013 21:28, Peter Lieven ha scritto:

Quoted 4 lines

thanks for the details. I think to have optimal performance and best change for unmapping in qemu-img convert
it might be best to export the OPTIMAL UNMAP GRANULARITY

Quoted 7 lines

Agreed about this.

Quoted 1 line

as well as the write_zeroes_w_discard capability via the BDI

But why this?!? It is _not_ needed. All you need is to change the default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is nonzero.

2 reasons:
a) Does this guarantee that the requests are aligned to multiple of the -S value?

b) If I this flag exists qemu-img convert can do the "zeroing" a priori. This
has the benefit that combined with bdrv_get_block_status requests before it might
not need to touch large areas of the volume. Speaking for iSCSI its likely that
the user sets a fresh volume as the destination, but its not guaranteed. With the Patch 4 there are only done a few get_block_status requests to verify
this. If we just write zeroes with BDRV_MAY_UNMAP, we send hundreds or thousands of writesame requests for possibly already unmapped data.

To give an example. If I take my storage with an 1TB volume. It takes about 10-12
get_block_status requests to verify that it is completely unmapped. After this
I am safe to set has_zero_init = 1 in qemu-img convert.

If I would convert a 1TB image to this target where lets say 50% are at leat 15MB
zero blocks (15MB is the OUG of my storage) it would take ~35000 write same requests to achieve the same.

Peter

Quoted 53 lines

Paolo

Quoted 50 lines

and than zero
out the whole device with bdrv_write_zeroes and the BDRV_MAY_DISCARD flag.

the logic for this is already prepared in patch4 (topic of this email). except that
i have to exchange bdrv_discard with bdrv_write_zeroes w/ BDRV_MAY_DISCARD.

what do you think?

Quoted 35 lines · Peter Lieven

Quoted 33 lines

Am 18.07.2013 um 16:35 schrieb Paolo Bonzini <[email protected]>:

Quoted 12 lines

Il 18/07/2013 16:32, Peter Lieven ha scritto:

Quoted 11 lines

Quoted 8 lines

(Mis)alignment and granularity can be handled later. We can ignore them
for now. Later, if we decide the best way to support them is a flag,
we'll add it. Let's not put the cart before the horse.

BTW, I expect alignment!=0 to be really, really rare.

To explain my concerns:

Quoted 8 lines

Quoted 6 lines

I know that my target has internal page size of 15MB. I will check what
happens
if I deallocate this 15MB in chunks of lets say 1MB. If the page gets unprovisioned
after the last chunk is unmapped it would be fine :-)

You're talking of granularity here, not (mis)alignment.

you are right. for the target i am talking about this is 30720 512-byte blocks for the granularity (pagesize) and 0 for the alignment.
i will see what happens if I write same w/unmap the whole 30720 blocks in smaller blocks ;-) otherwise i will have to add support for honoring this values in qemu-img convert
as a follow up.

Peter

--

Mit freundlichen Grüßen

Peter Lieven

...........................................................

KAMP Netzwerkdienste GmbH
Vestische Str. 89-91 | 46117 Oberhausen
Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40 [email protected] | http://www.kamp.de

Geschäftsführer: Heiner Lante | Michael Lante
Amtsgericht Duisburg | HRB Nr. 12154
USt-Id-Nr.: DE 120607556

...........................................................

Quoted 33 lines · Peter Lieven

Quoted 10 lines · Paolo Bonzini

Il 18/07/2013 21:28, Peter Lieven ha scritto:

Quoted 3 lines

thanks for the details. I think to have optimal performance and best change for unmapping in qemu-img convert
it might be best to export the OPTIMAL UNMAP GRANULARITY

Agreed about this.

Quoted 1 line

as well as the write_zeroes_w_discard capability via the BDI

But why this?!? It is _not_ needed. All you need is to change the default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is nonzero.

2 reasons:
a) Does this guarantee that the requests are aligned to multiple of the -S value?

b) If I this flag exists qemu-img convert can do the "zeroing" a priori. This
has the benefit that combined with bdrv_get_block_status requests before it might
not need to touch large areas of the volume. Speaking for iSCSI its likely that
the user sets a fresh volume as the destination, but its not guaranteed. With the Patch 4 there are only done a few get_block_status requests to verify
this. If we just write zeroes with BDRV_MAY_UNMAP, we send hundreds or thousands of writesame requests for possibly already unmapped data.

To give an example. If I take my storage with an 1TB volume. It takes about 10-12
get_block_status requests to verify that it is completely unmapped. After this
I am safe to set has_zero_init = 1 in qemu-img convert.

Quoted 4 lines

If I would convert a 1TB image to this target where lets say 50% are at leat 15MB
zero blocks (15MB is the OUG of my storage) it would take ~35000 write same requests to achieve the same.

I am not sure I am reading this right, but you dont have to writesame exactly 1xOUG to get it to unmap.
nxOUG will work too,
So instead of sending one writesame for each OUG range, you can send one writesame for every ~10G or so.
Say 10G is ~667 OUGs for your case, so you can send writesame for ~667xOUG in each command and then it would "only" take ~100 writesames instead of ~35000.

So as long as you are sending in multiples of OUG you should be fine.

Quoted 61 lines · ronnie sahlberg

Quoted 43 lines · Peter Lieven

Quoted 7 lines · Paolo Bonzini

Il 18/07/2013 21:28, Peter Lieven ha scritto:

Quoted 4 lines

thanks for the details. I think to have optimal performance and best change for unmapping in qemu-img convert
it might be best to export the OPTIMAL UNMAP GRANULARITY

Quoted 7 lines

Agreed about this.

Quoted 1 line

as well as the write_zeroes_w_discard capability via the BDI

But why this?!? It is _not_ needed. All you need is to change the default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is nonzero.

2 reasons:
a) Does this guarantee that the requests are aligned to multiple of the -S
value?

b) If I this flag exists qemu-img convert can do the "zeroing" a priori. This
has the benefit that combined with bdrv_get_block_status requests before it
might
not need to touch large areas of the volume. Speaking for iSCSI its likely
that
the user sets a fresh volume as the destination, but its not guaranteed. With the Patch 4 there are only done a few get_block_status requests to verify
this. If we just write zeroes with BDRV_MAY_UNMAP, we send hundreds or thousands of writesame requests for possibly already unmapped data.

To give an example. If I take my storage with an 1TB volume. It takes about
10-12
get_block_status requests to verify that it is completely unmapped. After this
I am safe to set has_zero_init = 1 in qemu-img convert.

Quoted 6 lines

If I would convert a 1TB image to this target where lets say 50% are at leat
15MB
zero blocks (15MB is the OUG of my storage) it would take ~35000 write same
requests to achieve the same.

I am not sure I am reading this right, but you dont have to writesame exactly 1xOUG to get it to unmap.
nxOUG will work too,
So instead of sending one writesame for each OUG range, you can send one writesame for every ~10G or so.
Say 10G is ~667 OUGs for your case, so you can send writesame for ~667xOUG in each command and then it would "only" take ~100 writesames instead of ~35000.

Quoted 1 line

So as long as you are sending in multiples of OUG you should be fine.

do I not have to take care of max_ws_size?

Peter

Quoted 54 lines · Peter Lieven

Quoted 53 lines · ronnie sahlberg

Quoted 37 lines · Peter Lieven

Quoted 10 lines · Paolo Bonzini

Il 18/07/2013 21:28, Peter Lieven ha scritto:

Quoted 3 lines

thanks for the details. I think to have optimal performance and best change for unmapping in qemu-img convert
it might be best to export the OPTIMAL UNMAP GRANULARITY

Agreed about this.

Quoted 1 line

as well as the write_zeroes_w_discard capability via the BDI

But why this?!? It is _not_ needed. All you need is to change the default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is nonzero.

2 reasons:
a) Does this guarantee that the requests are aligned to multiple of the -S
value?

b) If I this flag exists qemu-img convert can do the "zeroing" a priori. This
has the benefit that combined with bdrv_get_block_status requests before it
might
not need to touch large areas of the volume. Speaking for iSCSI its likely
that
the user sets a fresh volume as the destination, but its not guaranteed. With the Patch 4 there are only done a few get_block_status requests to verify
this. If we just write zeroes with BDRV_MAY_UNMAP, we send hundreds or thousands of writesame requests for possibly already unmapped data.

To give an example. If I take my storage with an 1TB volume. It takes about
10-12
get_block_status requests to verify that it is completely unmapped. After this
I am safe to set has_zero_init = 1 in qemu-img convert.

Quoted 6 lines

If I would convert a 1TB image to this target where lets say 50% are at leat
15MB
zero blocks (15MB is the OUG of my storage) it would take ~35000 write same
requests to achieve the same.

I am not sure I am reading this right, but you dont have to writesame exactly 1xOUG to get it to unmap.
nxOUG will work too,
So instead of sending one writesame for each OUG range, you can send one writesame for every ~10G or so.
Say 10G is ~667 OUGs for your case, so you can send writesame for ~667xOUG in each command and then it would "only" take ~100 writesames instead of ~35000.

Quoted 2 lines

Quoted 1 line

So as long as you are sending in multiples of OUG you should be fine.

do I not have to take care of max_ws_size?

Yes you need to handle mas_ws_size but I would imagine that on most
targets that max_ws_size >> OUG
I would be surprised if a target would set max_ws_size to just a single OUG.

Am 18.07.2013 um 16:35 schrieb Paolo Bonzini <[email protected]>:

Quoted 15 lines

Il 18/07/2013 16:32, Peter Lieven ha scritto:

Quoted 13 lines

Quoted 6 lines

Quoted 1 line

(Mis)alignment and granularity can be handled later. We can ignore them for now. Later, if we decide the best way to support them is a flag, we'll add it. Let's not put the cart before the horse.

BTW, I expect alignment!=0 to be really, really rare.

To explain my concerns:

I know that my target has internal page size of 15MB. I will check what happens
if I deallocate this 15MB in chunks of lets say 1MB. If the page gets unprovisioned
after the last chunk is unmapped it would be fine :-)

Quoted 1 line

You're talking of granularity here, not (mis)alignment.

you are right. for the target i am talking about this is 30720 512-byte blocks for the granularity (pagesize) and 0 for the alignment. i will see what happens if I write same w/unmap the whole 30720 blocks in smaller blocks ;-)

otherwise i will have to add support for honoring this values in qemu-img convert as a follow up.

Peter

Quoted 20 lines · Paolo Bonzini

Il 18/07/2013 13:04, Peter Lieven ha scritto:

Quoted 9 lines

Quoted 4 lines

But if you set BDRV_DISCARD_WRITE_ZEROES, then you always need a fallback to bdrv_write_zeroes. Why not just call bdrv_write_zeroes to begin with? That's why extending bdrv_write_zeroes is preferable.

In this case wo do not need a flag to the function at all. If the driver sets bdi->write_zeroes_w_discard = 1 then bdrv_write_zeroes can use bdrv_discard to write zeroes and the driver has to ensure that all is zero afterwards.

Peter, you removed exactly the part of the email where I explained the wrong part of your reasoning:

you cannot do that [discard in bdrv_write_zeroes] unconditionally. Some operations can use it, some cannot. Think of SCSI disk
emulation: it must not discard is WRITE SAME is sent without the UNMAP bit!

Quoted 6 lines

Quoted 2 lines

If the driver would have a better method of writing zeroes than discard it simply should not set bdi->write_zeroes_w_discard = 1.

If the driver had a better method of writing zeroes than discard, it simply should ignore the BDRV_MAY_UNMAP (or BDRV_MAY_DISCARD) flag in its bdrv_write_zeros implementation.

ok, but this would require an individual patch in every driver, wouldn't it. i am ok with that.

the BDRV_MAY_DISCARD flag is at the end a hint if the driver can optimize writing zeroes by a discard or if real zeroes should be written e.g. to sanitize the device?

talking for iscsi:
bdrv->discard can remain to use UNMAP and silently fail if lbpu == 0. bdrv->write_zeroes will use writesame16 and set the unmap flag only if BDRV_MAY_DISCARD == 1 and BDRV_O_UNMAP == 1 and lbprz == 1.

in case lbpws == 0 it will return -ENOSUP.

Correct?

Peter

Quoted 2 lines

Quoted 2 lines

bdrv->write_zeroes will use writesame16 and set the unmap flag only if BDRV_MAY_DISCARD == 1 and BDRV_O_UNMAP == 1 and lbprz == 1.

When you use WRITESAME16 you can ignore the lbprz flag.
Just send a WRITESAME16 command with one block of data that is all set to zero. If the unmap flag is set and if unmapped blocks read back the same as the block you provided (all zero)
then it will unmap those blocks, where possible.

All other blocks that can not be unmapped, or where unmapped blocks will not read back the same, will instead be overwritten by the provided all-zero block.

Il 18/07/2013 15:55, ronnie sahlberg ha scritto:

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

bdrv->write_zeroes will use writesame16 and set the unmap flag only if BDRV_MAY_DISCARD == 1 and BDRV_O_UNMAP == 1 and lbprz == 1.

When you use WRITESAME16 you can ignore the lbprz flag.
Just send a WRITESAME16 command with one block of data that is all set to zero. If the unmap flag is set and if unmapped blocks read back the same as the block you provided (all zero)
then it will unmap those blocks, where possible.

True, so the unmap flag can be set iff BDRV_MAY_DISCARD == 1. block.c can take care of checking BDRV_O_UNMAP.

Quoted 11 lines

Il 18/07/2013 15:55, ronnie sahlberg ha scritto:

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

bdrv->write_zeroes will use writesame16 and set the unmap flag only if BDRV_MAY_DISCARD == 1 and BDRV_O_UNMAP == 1 and lbprz == 1.

When you use WRITESAME16 you can ignore the lbprz flag. Just send a WRITESAME16 command with one block of data that is all set to zero.
If the unmap flag is set and if unmapped blocks read back the same as the block you provided (all zero)
then it will unmap those blocks, where possible.

True, so the unmap flag can be set iff BDRV_MAY_DISCARD == 1. block.c can take care of checking BDRV_O_UNMAP.

Would you add BDRV_MAY_DISCARD to BdrvRequestFlags?

This would require making BdrvRequestFlags public.

Before I start implementation I would still like to verify if we need this flag. E.g. in which case would we do not want to optimize writing zeroes via discard if the user has set BDRV_O_UNMAP?

One case I would think of is sanitizing data. But in this case shouldn't this
be a different function? In case of a container format its also not guaranteed that the
contents are erased from the underlying media. Looking at the commit message
for f08f2ddae078e8a7683f8b16da8e0cc3029c7b89 it sounds that bdrv_write_zeroes
was intended to provide an efficient way to zero contents, but only in the sense
that it is guaranteed that they read back as zero (compared to bdrv_discard).

Peter

Il 12/09/2013 10:52, Peter Lieven ha scritto:

Quoted 15 lines · Paolo Bonzini

Quoted 11 lines

Il 18/07/2013 15:55, ronnie sahlberg ha scritto:

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

bdrv->write_zeroes will use writesame16 and set the unmap flag only if BDRV_MAY_DISCARD == 1 and BDRV_O_UNMAP == 1 and lbprz == 1.

When you use WRITESAME16 you can ignore the lbprz flag. Just send a WRITESAME16 command with one block of data that is all set to zero.
If the unmap flag is set and if unmapped blocks read back the same as the block you provided (all zero)
then it will unmap those blocks, where possible.

True, so the unmap flag can be set iff BDRV_MAY_DISCARD == 1. block.c can take care of checking BDRV_O_UNMAP.

Would you add BDRV_MAY_DISCARD to BdrvRequestFlags?

This would require making BdrvRequestFlags public.

Quoted 4 lines

Before I start implementation I would still like to verify if we need this flag. E.g. in which case would we do not want to optimize writing zeroes via discard if the user has set BDRV_O_UNMAP?

For example if the bdrv_write_zeroes is coming from a WRITE SAME command (sent by the guest to an emulated SCSI disk). In this case, we should not discard if the UNMAP bit is zero in the request.

The WRITE SAME implementation in scsi-disk.c is old and broken, but the block layer should provide enough API to make it right.

For Disks we always use read/write16 so i think we Should also use writesame16. Or not?

Von meinem iPhone gesendet

Am 17.07.2013 um 18:31 schrieb ronnie sahlberg <[email protected]>:

Quoted 16 lines · Paolo Bonzini

Quoted 11 lines

Il 17/07/2013 17:54, ronnie sahlberg ha scritto:

Quoted 3 lines

I think it is reasonable to assume that IF LBPRZ==1 and IF it is an "optimal unmap request" then the blocks will become unmapped and they will read back as 0.

Yes, but it is not reasonable to assume that bdrv_discard will only receive "optimal" requests. Thus, using WRITE SAME for LBPRZ=1, and not exposing LBPRZ=1 if LBPWS=0 (may also use LBPWS10 depending on the capacity), is the safer thing to do.

Paolo

ACK.

WRITESAME10/16 with UNMAP flag set is probably best.

Quoted 19 lines

Am 17.07.2013 um 11:58 hat Paolo Bonzini geschrieben:

Quoted 2 lines

BTW, Peter and Ronnie: we were assuming that UNMAP with LBPRZ=1 always zeroes blocks, but is that true for unaligned operations?

SBC-3 Rev. 35e, 5.16 READ CAPACITY (16) command:

"A logical block provisioning read zeros (LBPRZ) bit set to one indicates that, for read commands specifying an unmapped LBA (see 4.7.4.5), the device server returns user data set to zero [...]"

So it depends on the block provisioning state of the LBA, not on the operations that were performed on it.

5.28 UNMAP command:

If the ANCHOR bit in the CDB is set to zero, and the logical unit is thin provisioned (see 4.7.3.3), then the logical block provisioning state for each specified LBA:

Quoted 8 lines

a) should become deallocated;
b) may become anchored; or
c) may remain unchanged.

So with UNMAP, I think you don't have any guarantees that the LBA becomes unmapped and therefore zeroed. It could just keep its current data. No matter whether your request was aligned or not.

If you check the LogicalBlockLimits VPD page it has :

---
[...]
An unmap request with a starting LBA that is not optimal may result in unmap operations on fewer LBAs than
requested.
---

So if Peter checks OPTIMAL_UNMAP_GRANULARITY and
UNMAP_GRANULARITY_ALIGNMENT and make sure that
the starting LBA is (n * OUG) + UGA and that the number of blocks
is a multiple of OUG then you should be fine.

I think it is reasonable to assume that IF LBPRZ==1 and IF it is an "optimal unmap request" then the blocks will become unmapped and they will
read back as 0.

Am 15.07.2013 um 12:49 hat Peter Lieven geschrieben:

Quoted 4 lines

if a destination has has_zero_init = 0, but it supports discard zeroes use discard to convert the target into an all zero device.

Quoted 1 line

Signed-off-by Peter Lieven <[email protected]>

Wouldn't it be better to use bdrv_write_zeroes() and extend the implementation of that to use discard internally in those block drivers where it makes sense?

Because here you're not really discarding (i.e. don't care about the sectors any more), but you want them to be zeroed.

this adds the proposed solution to add a generic mechanism to zeroize a target in qemu-img if it has discard_zeroes but has_zero_init is 0.

Peter Lieven (4):
block: add discard_zeroes and max_unmap to BlockDriverInfo
iscsi: add .bdrv_get_info
block/raw: add .bdrv_get_info
qemu-img: conditionally discard target on convert

 block/iscsi.c         |   19 +++++++++++++++--
 block/raw.c           |    6 ++++++
 include/block/block.h |    5 +++++
 qemu-img.c            |   56 ++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 83 insertions(+), 3 deletions(-)

Il 15/07/2013 12:49, Peter Lieven ha scritto:

Quoted 9 lines

this adds the proposed solution to add a generic mechanism to zeroize a target in qemu-img if it has discard_zeroes but has_zero_init is 0.

Peter Lieven (4):
block: add discard_zeroes and max_unmap to BlockDriverInfo
iscsi: add .bdrv_get_info
block/raw: add .bdrv_get_info
qemu-img: conditionally discard target on convert

Quoted 6 lines

  block/iscsi.c         |   19 +++++++++++++++--
  block/raw.c           |    6 ++++++
  include/block/block.h |    5 +++++
  qemu-img.c            |   56 ++++++++++++++++++++++++++++++++++++++++++++++++-
  4 files changed, 83 insertions(+), 3 deletions(-)

Looks good. Perhaps, as a followup, you can use max_unmap (which I'd rename to max_discard) in bdrv_co_discard to support splitting the request. You could also export the discard alignment in BlockDriverInfo.

Quoted 3 lines

Looks good. Perhaps, as a followup, you can use max_unmap (which I'd rename to max_discard) in bdrv_co_discard to support splitting the request. You could also export the discard alignment in BlockDriverInfo.

I already did this in my previous series for iscsi.
But I can also add a follow
up if in the future another driver specifies max_discard. I will drop it from iscsi again then.

Before this patch here is merged, we need the other series "[PATCHv3 00/10] iscsi/qemu-img/block-migration enhancements" otherwise patch 2/4 will not apply. Maybe you can put it together again or I will do a v4 with all patches together.

Il 16/07/2013 13:18, Peter Lieven ha scritto:

Quoted 7 lines

Quoted 3 lines

Looks good. Perhaps, as a followup, you can use max_unmap (which I'd rename to max_discard) in bdrv_co_discard to support splitting the request. You could also export the discard alignment in BlockDriverInfo.

I already did this in my previous series for iscsi.
But I can also add a follow
up if in the future another driver specifies max_discard. I will drop it from iscsi again then.

In general, I prefer to have things in the block layer if there is a chance of them becoming generic. I guess in this case the chance is relatively small.

On the other hand, if you add any smartness such as handling alignment (see http://lkml.org/lkml/2012/7/5/342) please do take the occasion to move it out of block/iscsi.c.

Quoted 5 lines

Before this patch here is merged, we need the other series "[PATCHv3 00/10] iscsi/qemu-img/block-migration enhancements" otherwise patch 2/4 will not apply. Maybe you can put it together again or I will do a v4 with all patches together.

Perhaps. Both might also conflict with my get_block_status patches. I'm going to send an updated version today. Ugh, it's already 1:30 PM. :(

The conflict with your block status patches can't be large. upstream/master has no
iscsi_co_is_allocated yet, so there should be no trouble.

If you find no objections merging "[PATCHv3 00/10] iscsi/qemu-img/block-migration enhancements"
first would be nice. Patch 06/10 and 10/10 are important fixes.

Il 16/07/2013 13:40, Peter Lieven ha scritto:

Quoted 4 lines

The conflict with your block status patches can't be large. upstream/master has no
iscsi_co_is_allocated yet, so there should be no trouble.

Yes, whoever goes second has to change it to the new get_block_status API. Kevin and Stefan can decide who that lucky guy is.

Am 16.07.2013 um 13:55 schrieb Paolo Bonzini <[email protected]>:

Quoted 2 lines

Il 16/07/2013 13:40, Peter Lieven ha scritto:

Quoted 1 line

Quoted 6 lines

Quoted 3 lines

The conflict with your block status patches can't be large. upstream/master has no
iscsi_co_is_allocated yet, so there should be no trouble.

Yes, whoever goes second has to change it to the new get_block_status API. Kevin and Stefan can decide who that lucky guy is.

Wouldn't you be able to merge it into scsi/next ?

Il 17/07/2013 12:23, Peter Lieven ha scritto:

Quoted 9 lines

Am 16.07.2013 um 13:55 schrieb Paolo Bonzini <[email protected]>:

Quoted 6 lines

Il 16/07/2013 13:40, Peter Lieven ha scritto:

Quoted 4 lines

The conflict with your block status patches can't be large. upstream/master has no
iscsi_co_is_allocated yet, so there should be no trouble.

Quoted 4 lines

Quoted 2 lines

Yes, whoever goes second has to change it to the new get_block_status API. Kevin and Stefan can decide who that lucky guy is.

Wouldn't you be able to merge it into scsi/next ?

get_block_status is not work for the SCSI tree. If you mean making it easier for you to rebase on top, the v2 I posted yesterday is in branch block-flags in my github repo.

Am 17.07.2013 um 12:28 schrieb Paolo Bonzini <[email protected]>:

Quoted 15 lines

Il 17/07/2013 12:23, Peter Lieven ha scritto:

Quoted 13 lines

Am 16.07.2013 um 13:55 schrieb Paolo Bonzini <[email protected]>:

Quoted 8 lines

Il 16/07/2013 13:40, Peter Lieven ha scritto:

Quoted 4 lines

The conflict with your block status patches can't be large. upstream/master has no
iscsi_co_is_allocated yet, so there should be no trouble.

Yes, whoever goes second has to change it to the new get_block_status API. Kevin and Stefan can decide who that lucky guy is.

Wouldn't you be able to merge it into scsi/next ?

Quoted 3 lines

get_block_status is not work for the SCSI tree. If you mean making it easier for you to rebase on top, the v2 I posted yesterday is in branch block-flags in my github repo.

That would be ok if the patches are merged first. Otherwise I could ask Kevin to merge my old series (except the iscsi_co_write_zeroes patch as there obviously is still room for discussion and improvement) and you could tweak iscsi_co_is_allocated later?

Il 17/07/2013 12:40, Peter Lieven ha scritto:

Quoted 22 lines

Am 17.07.2013 um 12:28 schrieb Paolo Bonzini <[email protected]>:

Quoted 18 lines

Il 17/07/2013 12:23, Peter Lieven ha scritto:

Quoted 5 lines

Am 16.07.2013 um 13:55 schrieb Paolo Bonzini <[email protected]>:

Quoted 2 lines

Il 16/07/2013 13:40, Peter Lieven ha scritto:

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

The conflict with your block status patches can't be large. upstream/master has no
iscsi_co_is_allocated yet, so there should be no trouble.

Yes, whoever goes second has to change it to the new get_block_status API. Kevin and Stefan can decide who that lucky guy is.

Wouldn't you be able to merge it into scsi/next ?

get_block_status is not work for the SCSI tree. If you mean making it easier for you to rebase on top, the v2 I posted yesterday is in branch block-flags in my github repo.

Quoted 4 lines

That would be ok if the patches are merged first. Otherwise I could ask Kevin to merge my old series (except the iscsi_co_write_zeroes patch as there obviously is still room for discussion and improvement) and you could tweak iscsi_co_is_allocated later?

I'll look at your old series, I think the conflicts are relatively trivial. But I think that this series must wait for 1.7.

Paolo

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

That would be ok if the patches are merged first. Otherwise I could ask Kevin to merge my old series (except the iscsi_co_write_zeroes patch as there obviously is still room for discussion and improvement) and you could tweak iscsi_co_is_allocated later?

I'll look at your old series, I think the conflicts are relatively trivial. But I think that this series must wait for 1.7.

Would it be an Option to merge it except for the iscsi_co_is_allocated and the iscsi_co_write_zeroes patch. 3 of the Patches fix potential bugs.

Il 17/07/2013 16:18, Peter Lieven ha scritto:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

That would be ok if the patches are merged first. Otherwise I could ask Kevin to merge my old series (except the iscsi_co_write_zeroes patch as there obviously is still room for discussion and improvement) and you could tweak iscsi_co_is_allocated later?

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

I'll look at your old series, I think the conflicts are relatively trivial. But I think that this series must wait for 1.7.

Would it be an Option to merge it except for the iscsi_co_is_allocated and the iscsi_co_write_zeroes patch. 3 of the Patches fix potential bugs.

Yes, of course! And I think I can merge them via scsi/next, too. What patches are you thinking of exactly? Can you write the numbers?

Am 17.07.2013 um 16:26 schrieb Paolo Bonzini <[email protected]>:

Quoted 10 lines

Il 17/07/2013 16:18, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

That would be ok if the patches are merged first. Otherwise I could ask Kevin to merge my old series (except the iscsi_co_write_zeroes patch as there obviously is still room for discussion and improvement) and you could tweak iscsi_co_is_allocated later?

I'll look at your old series, I think the conflicts are relatively trivial. But I think that this series must wait for 1.7.

Would it be an Option to merge it except for the iscsi_co_is_allocated and the iscsi_co_write_zeroes patch. 3 of the Patches fix potential bugs.

Quoted 2 lines

Yes, of course! And I think I can merge them via scsi/next, too. What patches are you thinking of exactly? Can you write the numbers?

Mandatory:
[PATCHv3 07/10] iscsi: fix -ENOSPC in iscsi_create()
[PATCHv3 08/10] iscsi: factor out sector conversions
[PATCHv3 09/10] iscsi: remove support for misaligned nb_sectors in aio_readv [PATCHv3 10/10] iscsi: assert that sectors are aligned to LUN blocksize

Optional (non-conflicting patches):
[PATCHv3 01/10] iscsi: add logical block provisioning information to iscsilun [PATCHv3 05/10] block: add bdrv_write_zeroes()
[PATCHv3 06/10] block/raw: add bdrv_co_write_zeroes

Il 17/07/2013 16:46, Peter Lieven ha scritto:

Quoted 13 lines

Am 17.07.2013 um 16:26 schrieb Paolo Bonzini <[email protected]>:

Quoted 10 lines

Il 17/07/2013 16:18, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

That would be ok if the patches are merged first. Otherwise I could ask Kevin to merge my old series (except the iscsi_co_write_zeroes patch as there obviously is still room for discussion and improvement) and you could tweak iscsi_co_is_allocated later?

I'll look at your old series, I think the conflicts are relatively trivial. But I think that this series must wait for 1.7.

Would it be an Option to merge it except for the iscsi_co_is_allocated and the iscsi_co_write_zeroes patch. 3 of the Patches fix potential bugs.

Quoted 8 lines

Quoted 2 lines

Yes, of course! And I think I can merge them via scsi/next, too. What patches are you thinking of exactly? Can you write the numbers?

Mandatory:
[PATCHv3 07/10] iscsi: fix -ENOSPC in iscsi_create()
[PATCHv3 08/10] iscsi: factor out sector conversions
[PATCHv3 09/10] iscsi: remove support for misaligned nb_sectors in aio_readv [PATCHv3 10/10] iscsi: assert that sectors are aligned to LUN blocksize

Applied all to scsi-next (trivial conflict in patches 8 and 10, solved them myself---just be careful when rebasing).

Quoted 4 lines

Optional (non-conflicting patches):
[PATCHv3 01/10] iscsi: add logical block provisioning information to iscsilun [PATCHv3 05/10] block: add bdrv_write_zeroes()
[PATCHv3 06/10] block/raw: add bdrv_co_write_zeroes

5 and 6 must go through Kevin.

Am 17.07.2013 um 16:53 hat Paolo Bonzini geschrieben:

Quoted 31 lines

Il 17/07/2013 16:46, Peter Lieven ha scritto:

Quoted 13 lines

Am 17.07.2013 um 16:26 schrieb Paolo Bonzini <[email protected]>:

Quoted 10 lines

Il 17/07/2013 16:18, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

That would be ok if the patches are merged first. Otherwise I could ask Kevin to merge my old series (except the iscsi_co_write_zeroes patch as there obviously is still room for discussion and improvement) and you could tweak iscsi_co_is_allocated later?

I'll look at your old series, I think the conflicts are relatively trivial. But I think that this series must wait for 1.7.

Would it be an Option to merge it except for the iscsi_co_is_allocated and the iscsi_co_write_zeroes patch. 3 of the Patches fix potential bugs.

Quoted 8 lines

Quoted 2 lines

Yes, of course! And I think I can merge them via scsi/next, too. What patches are you thinking of exactly? Can you write the numbers?

Mandatory:
[PATCHv3 07/10] iscsi: fix -ENOSPC in iscsi_create()
[PATCHv3 08/10] iscsi: factor out sector conversions
[PATCHv3 09/10] iscsi: remove support for misaligned nb_sectors in aio_readv [PATCHv3 10/10] iscsi: assert that sectors are aligned to LUN blocksize

Applied all to scsi-next (trivial conflict in patches 8 and 10, solved them myself---just be careful when rebasing).

Quoted 4 lines

Optional (non-conflicting patches):
[PATCHv3 01/10] iscsi: add logical block provisioning information to iscsilun [PATCHv3 05/10] block: add bdrv_write_zeroes()
[PATCHv3 06/10] block/raw: add bdrv_co_write_zeroes

Quoted 1 line

5 and 6 must go through Kevin.

This week is Stefan's.

But didn't I give a Reviewed-by for an earlier version of these patches, Peter? It doesn't seem to have made its way to the commit message.

Kevin

Il 17/07/2013 16:46, Peter Lieven ha scritto:

Quoted 13 lines

Am 17.07.2013 um 16:26 schrieb Paolo Bonzini <[email protected]>:

Quoted 10 lines

Il 17/07/2013 16:18, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

That would be ok if the patches are merged first. Otherwise I could ask Kevin to merge my old series (except the iscsi_co_write_zeroes patch as there obviously is still room for discussion and improvement) and you could tweak iscsi_co_is_allocated later?

I'll look at your old series, I think the conflicts are relatively trivial. But I think that this series must wait for 1.7.

Would it be an Option to merge it except for the iscsi_co_is_allocated and the iscsi_co_write_zeroes patch. 3 of the Patches fix potential bugs.

Quoted 8 lines

Quoted 2 lines

Yes, of course! And I think I can merge them via scsi/next, too. What patches are you thinking of exactly? Can you write the numbers?

Mandatory:
[PATCHv3 07/10] iscsi: fix -ENOSPC in iscsi_create()
[PATCHv3 08/10] iscsi: factor out sector conversions
[PATCHv3 09/10] iscsi: remove support for misaligned nb_sectors in aio_readv [PATCHv3 10/10] iscsi: assert that sectors are aligned to LUN blocksize

Applied all to scsi-next (trivial conflict in patches 8 and 10, solved them myself---just be careful when rebasing).

Quoted 4 lines

Optional (non-conflicting patches):
[PATCHv3 01/10] iscsi: add logical block provisioning information to iscsilun [PATCHv3 05/10] block: add bdrv_write_zeroes()
[PATCHv3 06/10] block/raw: add bdrv_co_write_zeroes

5 and 6 must go through Kevin. As to 1, I suspect if you redo discard along the lines we discussed today it would become heavily different, so I'd rather not take it too.

Paolo

this adds the proposed solution to add a generic mechanism to zeroize a target in qemu-img if it has discard_zeroes but has_zero_init is 0.

Peter Lieven (4):
block: add discard_zeroes and max_unmap to BlockDriverInfo
iscsi: add .bdrv_get_info
block/raw: add .bdrv_get_info
qemu-img: conditionally discard target on convert

 block/iscsi.c         |   19 +++++++++++++++--
 block/raw.c           |    6 ++++++
 include/block/block.h |    5 +++++
 qemu-img.c            |   56 ++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 83 insertions(+), 3 deletions(-)