Re: RFC: 32-bit __data_len and REQ_DISCARD+REQ_SECURE

[Grant Grundler]

On Tue, Oct 20, 2015 at 11:57 AM, Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
> Hi Grant,
>
> Grant Grundler <grundler@xxxxxxxxxxxx> writes:
>
>> Ping? Does no one care how long BLK_SECDISCARD takes?
>>
>> ChromeOS has landed this change as a compromise between "fast" (<10
>> seconds) and "minimize risk" (~90 seconds) for a 23GB partition on
>> eMMC:
>>     https://chromium-review.googlesource.com/#/c/302413/
>
> Including the patch would be helpful.  I believe this is it.

Thanks Jeff!  Gerrit does provide easy mechanisms to review or pull
the patch - easy to use - not easy to find though. :/

> My comments are inline.
>
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 8411be3..43943c7 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
>
> @@ -60,21 +60,37 @@
>         granularity = max(q->limits.discard_granularity >> 9, 1U);
>         alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;
>
> -       /*
> -        * Ensure that max_discard_sectors is of the proper
> -        * granularity, so that requests stay aligned after a split.
> -        */
> -       max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9);
> -       max_discard_sectors -= max_discard_sectors % granularity;
> -       if (unlikely(!max_discard_sectors)) {
> -               /* Avoid infinite loop below. Being cautious never hurts. */
> -               return -EOPNOTSUPP;
> -       }
> +       max_discard_sectors = min(q->limits.max_discard_sectors,
> +                                               UINT_MAX >> 9);
>
> Unnecessary reformatting.
>
>         if (flags & BLKDEV_DISCARD_SECURE) {
>                 if (!blk_queue_secdiscard(q))
>                         return -EOPNOTSUPP;
>                 type |= REQ_SECURE;
> +               /*
> +                * Secure erase performs better by telling the device
> +                * about the largest range possible.  Secure erase
> +                * piecemeal will likely result in mapped sectors
> +                * getting evacuated from one range and parked in
> +                * another range that will get erased by a future
> +                * erase command.  This does NOT happen for normal
> +                * TRIM or DISCARD operations.
> +                *
> +                * 32GB was a compromise to avoid blocking the device
> +                * for potentially minute(s) at a time.
> +                */
> +               if (max_discard_sectors < (1 << (25-9)))        /* 32GiB */
> +                       max_discard_sectors = 1 << (25-9);
>
> And here you're ignoring q->limits.max_discard_sectors.  I'm surprised
> this worked!

See Gwendal's earlier reply. Here is the entire thread:
   https://lkml.org/lkml/2015/9/22/1235

>
> +       }
> +
> +       /*
> +        * Ensure that max_discard_sectors is of the proper
> +        * granularity, so that requests stay aligned after a split.
> +        */
> +       max_discard_sectors -= max_discard_sectors % granularity;
> +       if (unlikely(!max_discard_sectors)) {
> +               /* Avoid infinite loop below. Being cautious never hurts. */
> +               return -EOPNOTSUPP;
>         }
>
>         atomic_set(&bb.done, 1);
>
> Grant, can we start over with the problem description? (Sorry, I didn't
> see the previous posts.)

First/second posting in https://lkml.org/lkml/2015/9/22/1235 should
provide this.

>  I'd like to know the values of discard_granularity
> and discard_max_bytes for your device.

Gwendal might be able to provide those. I no longer have possession the HW.

>  Additionally, it would be
> interesting to know how the discards are being initiatied.  Is it via a
> userspace utility such as mkfs, online discard via some file system
> mounted with -o discard, or something else?

BLK_SECDISCARD ioctl with parameters to describe the /data partition
on an android device.

> Finally, can you post
> binary blktrace data somewhere for the slow case?

Sorry,  -ENOHW.
 second
I only have a snippet of printk output from the original code with
slow performance:
[   13.409334] sdhci-cmd:  CMD 0x231a arg 0x3976800
[   13.414150] sdhci-cmd:  CMD 0x241a arg 0x3976bff
[   13.418790] sdhci-cmd:  CMD 0x261b arg 0x80000000
[   13.424488] sdhci-cmd:  CMD 0xd1a arg 0x10000
[   13.429622] sdhci-cmd:  CMD 0x231a arg 0x3976c00
[   13.434333] sdhci-cmd:  CMD 0x241a arg 0x3976fff
[   13.438968] sdhci-cmd:  CMD 0x261b arg 0x80000000
[   13.443717] sdhci-cmd:  CMD 0xd1a arg 0x10000
[   13.448113] sdhci-cmd:  CMD 0x231a arg 0x3977000
[   13.453087] sdhci-cmd:  CMD 0x241a arg 0x39773ff
[   13.457780] sdhci-cmd:  CMD 0x261b arg 0x80000000
[   13.462839] sdhci-cmd:  CMD 0xd1a arg 0x10000
[   13.468237] sdhci-cmd:  CMD 0x231a arg 0x3977400
[   13.472980] sdhci-cmd:  CMD 0x241a arg 0x39777ff
[   13.477619] sdhci-cmd:  CMD 0x261b arg 0x80000000
[   13.482352] sdhci-cmd:  CMD 0xd1a arg 0x10000

"CMD" is 35/36/38/13 (but in hex) + flags (IIRC)

Each command is taking ~20ms. But multiple that by 46k to erase the
entire 23GB partition == 15 minutes.

I will assert this is "best case" since I usually tested with very
little "live" data (< 300MB) that would need to be evacuated from any
given erase block.

> Thanks!

Thanks for the feedback! :)

cheers,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/