blkdev: Submit discard bio in batches in blkdev_issue_discard()

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> As Jeff Moyer pointed out we do not honor discard granularity Lukas> while submitting REQ_DISCARD bios of size smaller than Lukas> max_discard_sectors. That fact might have unwanted consequences Lukas> of device ignoring the request, or even worse if device firmware Lukas> is buggy.

We've discussed this before and the consensus was not to do it. The granularity is a hint, not a hard limit like max_discard_sectors.

We want the reporting to be comprehensive throughout the block layer. If we start aligning to the granularity at the top we lose information for stacked devices below with a finer granularity.

So if we were to align to the granularity we'd want to do it at the bottom of the stack when we issue the command to the device. We've had a few proposed patches to did that but so far we've only found one device where it made a difference. And that case didn't justify adding a quirk.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Jeff" == Jeff Moyer <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> As Jeff Moyer pointed out we do not honor discard granularity Lukas> while submitting REQ_DISCARD bios of size smaller than Lukas> max_discard_sectors. That fact might have unwanted consequences Lukas> of device ignoring the request, or even worse if device firmware Lukas> is buggy.

We've discussed this before and the consensus was not to do it. The granularity is a hint, not a hard limit like max_discard_sectors.

We want the reporting to be comprehensive throughout the block layer. If we start aligning to the granularity at the top we lose information for stacked devices below with a finer granularity.

So if we were to align to the granularity we'd want to do it at the bottom of the stack when we issue the command to the device. We've had a few proposed patches to did that but so far we've only found one device where it made a difference. And that case didn't justify adding a quirk.

Currently we are waiting for every submitted REQ_DISCARD bio separately, but it can have unwanted consequences of repeatedly flushing the queue, so we rather submit bios in batches and wait for the entire batch, hence narrowing the window of other ios going in.

Use bio_batch_end_io() and struct bio_batch for that purpose, the same is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we always set !BIO_UPTODATE in the case of error and remove the check for bb, since we are the only user of this function and we always set this.

Remove bio_get()/bio_put() from the blkdev_issue_discard() since bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is not needed anymore.

I have done simple dd testing with surprising results. The script I have used is:

for i in $(seq 10); do

        echo $i
        dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
        sleep 5

done
/usr/bin/time -f %e ./blkdiscard /dev/sdc1

Running time of BLKDISCARD on the whole device:
with patch without patch
0.95 15.58

So we can see that in this artificial test the kernel with the patch applied is approx 16x faster in discarding the device.

Quoted 28 lines · Lukas Czerner

Currently we are waiting for every submitted REQ_DISCARD bio separately, but it can have unwanted consequences of repeatedly flushing the queue, so we rather submit bios in batches and wait for the entire batch, hence narrowing the window of other ios going in.

Use bio_batch_end_io() and struct bio_batch for that purpose, the same is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we always set !BIO_UPTODATE in the case of error and remove the check for bb, since we are the only user of this function and we always set this.

Remove bio_get()/bio_put() from the blkdev_issue_discard() since bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is not needed anymore.

I have done simple dd testing with surprising results. The script I have used is:

for i in $(seq 10); do

        echo $i
        dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
        sleep 5

done
/usr/bin/time -f %e ./blkdiscard /dev/sdc1

Running time of BLKDISCARD on the whole device:
with patch without patch
0.95 15.58

Quoted 2 lines

So we can see that in this artificial test the kernel with the patch applied is approx 16x faster in discarding the device.

Good results. It'd be more efficient to add the vectored discard support and use it for this too though, and it would get rid of the need to wait on the batches since there'd be just one discard bio for the ranges.

I know that you and Christoph were working on adding support for vectored discard, but I do not know what is the status of it ?

Quoted 32 lines · Jens Axboe

Quoted 30 lines · Lukas Czerner

Currently we are waiting for every submitted REQ_DISCARD bio separately, but it can have unwanted consequences of repeatedly flushing the queue, so we rather submit bios in batches and wait for the entire batch, hence narrowing the window of other ios going in.

Use bio_batch_end_io() and struct bio_batch for that purpose, the same is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we always set !BIO_UPTODATE in the case of error and remove the check for bb, since we are the only user of this function and we always set this.

Remove bio_get()/bio_put() from the blkdev_issue_discard() since bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is not needed anymore.

I have done simple dd testing with surprising results. The script I have used is:

for i in $(seq 10); do

        echo $i
        dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
        sleep 5

done
/usr/bin/time -f %e ./blkdiscard /dev/sdc1

Running time of BLKDISCARD on the whole device:
with patch without patch
0.95 15.58

So we can see that in this artificial test the kernel with the patch applied is approx 16x faster in discarding the device.

Quoted 4 lines

Good results. It'd be more efficient to add the vectored discard support and use it for this too though, and it would get rid of the need to wait on the batches since there'd be just one discard bio for the ranges.

I know that you and Christoph were working on adding support for vectored discard, but I do not know what is the status of it ?

Quoted 28 lines · Lukas Czerner

Currently we are waiting for every submitted REQ_DISCARD bio separately, but it can have unwanted consequences of repeatedly flushing the queue, so we rather submit bios in batches and wait for the entire batch, hence narrowing the window of other ios going in.

Use bio_batch_end_io() and struct bio_batch for that purpose, the same is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we always set !BIO_UPTODATE in the case of error and remove the check for bb, since we are the only user of this function and we always set this.

Remove bio_get()/bio_put() from the blkdev_issue_discard() since bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is not needed anymore.

I have done simple dd testing with surprising results. The script I have used is:

for i in $(seq 10); do

        echo $i
        dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
        sleep 5

done
/usr/bin/time -f %e ./blkdiscard /dev/sdc1

Running time of BLKDISCARD on the whole device:
with patch without patch
0.95 15.58

Quoted 2 lines

So we can see that in this artificial test the kernel with the patch applied is approx 16x faster in discarding the device.

Good results. It'd be more efficient to add the vectored discard support and use it for this too though, and it would get rid of the need to wait on the batches since there'd be just one discard bio for the ranges.

Quoted 32 lines · Jens Axboe

Quoted 30 lines · Lukas Czerner

Currently we are waiting for every submitted REQ_DISCARD bio separately, but it can have unwanted consequences of repeatedly flushing the queue, so we rather submit bios in batches and wait for the entire batch, hence narrowing the window of other ios going in.

Use bio_batch_end_io() and struct bio_batch for that purpose, the same is used by blkdev_issue_zeroout(). Also change bio_batch_end_io() so we always set !BIO_UPTODATE in the case of error and remove the check for bb, since we are the only user of this function and we always set this.

Remove bio_get()/bio_put() from the blkdev_issue_discard() since bio_alloc() and bio_batch_end_io() is doing the same thing, hence it is not needed anymore.

I have done simple dd testing with surprising results. The script I have used is:

for i in $(seq 10); do

        echo $i
        dd if=/dev/sdb1 of=/dev/sdc1 bs=4k &
        sleep 5

done
/usr/bin/time -f %e ./blkdiscard /dev/sdc1

Running time of BLKDISCARD on the whole device:
with patch without patch
0.95 15.58

So we can see that in this artificial test the kernel with the patch applied is approx 16x faster in discarding the device.

Quoted 4 lines

Good results. It'd be more efficient to add the vectored discard support and use it for this too though, and it would get rid of the need to wait on the batches since there'd be just one discard bio for the ranges.

Since we do not have vectored discard yet, can this change go in as it is ? Also, Christoph thinks that it would not help anyway since the we do batch the discard requests only when it exceeds the max range.

Quoted 4 lines

Good results. It'd be more efficient to add the vectored discard support and use it for this too though, and it would get rid of the need to wait on the batches since there'd be just one discard bio for the ranges.

Since we do not have vectored discard yet, can this change go in as it is ? Also, Christoph thinks that it would not help anyway since the we do batch the discard requests only when it exceeds the max range.

Currently we return -EOPNOTSUPP in blkdev_issue_discard() if any of the bio fails due to underlying device not supporting discard request. However, if the device is for example dm device composed of devices which some of them support discard and some of them does not, it is ok for some bios to fail with EOPNOTSUPP, but it does not mean that discard is not supported at all.

This commit removes the check for bios failed with EOPNOTSUPP and change blkdev_issue_discard() to return operation not supported if and only if the device does not actually supports it, not just part of the device as some bios might indicate.

This change also fixes problem with BLKDISCARD ioctl() which now works correctly on such dm devices.