On 6/25/21 9:30 AM, yanhuan916@xxxxxxxxx wrote:
> From: Huan Yan <yanhuan916@xxxxxxxxx>
>
> This patch make --discard option more flexible, it can be indicated after each
> backing or cache device.
NACK.
Reason 1, Only cache device requires discard in bcache-tools. Backing
device will be handled by upper layer like mkfs.
Reason 2, the patch format does not follow current bcache-tools patch
format style.
Coly Li
> ---
> make.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 48 insertions(+), 14 deletions(-)
>
> diff --git a/make.c b/make.c
> index 34d8011..39b381a 100644
> --- a/make.c
> +++ b/make.c
> @@ -402,6 +402,18 @@ static void write_sb(char *dev, struct sb_context *sbc, bool bdev, bool force)
> (unsigned int) sb.version,
> sb.block_size,
> data_offset);
> +
> + /* Attempting to discard backing device
> + */
> + if (discard) {
> + if (blkdiscard_all(dev, fd) < 0) {
> + fprintf(stderr,
> + "Failed to discard device %s, %s\n",
> + dev, strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> + }
> +
> putchar('\n');
> } else {
> if (nvdimm_meta)
> @@ -446,8 +458,15 @@ static void write_sb(char *dev, struct sb_context *sbc, bool bdev, bool force)
>
> /* Attempting to discard cache device
> */
> - if (discard)
> - blkdiscard_all(dev, fd);
> + if (discard) {
> + if (blkdiscard_all(dev, fd) < 0) {
> + fprintf(stderr,
> + "Failed to discard device %s, %s\n",
> + dev, strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> + }
> +
> putchar('\n');
> }
>
> @@ -660,21 +679,27 @@ static unsigned int get_blocksize(const char *path)
> int make_bcache(int argc, char **argv)
> {
> int c;
> - unsigned int i;
> + unsigned int i, n;
> int cdev = -1, bdev = -1, mdev = -1;
> unsigned int ncache_devices = 0, ncache_nvm_devices = 0;
> unsigned int nbacking_devices = 0;
> char *cache_devices[argc];
> char *cache_nvm_devices[argc];
> char *backing_devices[argc];
> + bool cache_devices_discard[argc];
> + bool backing_devices_discard[argc];
> + bool *device_discard = NULL;
> char label[SB_LABEL_SIZE] = { 0 };
> unsigned int block_size = 0, bucket_size = 1024;
> - int writeback = 0, discard = 0, wipe_bcache = 0, force = 0;
> + int writeback = 0, wipe_bcache = 0, force = 0;
> unsigned int cache_replacement_policy = 0;
> uint64_t data_offset = BDEV_DATA_START_DEFAULT;
> uuid_t set_uuid;
> struct sb_context sbc;
>
> + memset(cache_devices_discard, 0, sizeof(cache_devices_discard));
> + memset(backing_devices_discard, 0, sizeof(backing_devices_discard));
> +
> uuid_generate(set_uuid);
>
> struct option opts[] = {
> @@ -685,10 +710,8 @@ int make_bcache(int argc, char **argv)
> { "block", 1, NULL, 'w' },
> { "writeback", 0, &writeback, 1 },
> { "wipe-bcache", 0, &wipe_bcache, 1 },
> - { "discard", 0, &discard, 1 },
> - { "cache_replacement_policy", 1, NULL, 'p' },
> + { "discard", 0, NULL, 'd' },
> { "cache-replacement-policy", 1, NULL, 'p' },
> - { "data_offset", 1, NULL, 'o' },
> { "data-offset", 1, NULL, 'o' },
> { "cset-uuid", 1, NULL, 'u' },
> { "help", 0, NULL, 'h' },
> @@ -710,6 +733,10 @@ int make_bcache(int argc, char **argv)
> case 'M':
> mdev = 1;
> break;
> + case 'd':
> + if (device_discard)
> + *device_discard = true;
> + break;
> case 'b':
> bucket_size =
> hatoi_validate(optarg, "bucket size", UINT_MAX);
> @@ -762,15 +789,20 @@ int make_bcache(int argc, char **argv)
> }
>
> if (bdev > 0) {
> - backing_devices[nbacking_devices++] = optarg;
> - printf("backing_devices[%d]: %s\n", nbacking_devices - 1, optarg);
> + n = nbacking_devices++;
> + backing_devices[n] = optarg;
> + printf("backing_devices[%d]: %s\n", n, optarg);
> + device_discard = &backing_devices_discard[n];
> bdev = -1;
> } else if (cdev > 0) {
> - cache_devices[ncache_devices++] = optarg;
> - printf("cache_devices[%d]: %s\n", ncache_devices - 1, optarg);
> + n = ncache_devices++;
> + cache_devices[n] = optarg;
> + printf("cache_devices[%d]: %s\n", n, optarg);
> + device_discard = &cache_devices_discard[n];
> cdev = -1;
> } else if (mdev > 0) {
> - cache_nvm_devices[ncache_nvm_devices++] = optarg;
> + n = ncache_nvm_devices++;
> + cache_nvm_devices[n] = optarg;
> mdev = -1;
> }
> break;
> @@ -806,7 +838,6 @@ int make_bcache(int argc, char **argv)
> sbc.block_size = block_size;
> sbc.bucket_size = bucket_size;
> sbc.writeback = writeback;
> - sbc.discard = discard;
> sbc.wipe_bcache = wipe_bcache;
> sbc.cache_replacement_policy = cache_replacement_policy;
> sbc.data_offset = data_offset;
> @@ -814,13 +845,16 @@ int make_bcache(int argc, char **argv)
> sbc.label = label;
> sbc.nvdimm_meta = (ncache_nvm_devices > 0) ? true : false;
>
> - for (i = 0; i < ncache_devices; i++)
> + for (i = 0; i < ncache_devices; i++) {
> + sbc.discard = cache_devices_discard[i];
> write_sb(cache_devices[i], &sbc, false, force);
> + }
>
> for (i = 0; i < nbacking_devices; i++) {
> check_data_offset_for_zoned_device(backing_devices[i],
> &sbc.data_offset);
>
> + sbc.discard = backing_devices_discard[i];
> write_sb(backing_devices[i], &sbc, true, force);
> }
>
