Re: [PATCH] bcache: add a new sysfs interface to disable refill when read miss

[Coly Li <colyli@suse.de>]

On 2020/7/14 13:57, Hannes Reinecke wrote:
> On 7/13/20 5:28 AM, Guoju Fang wrote:
>> When read cache miss, backing device will be read first, and then refill
>> the cache device. But under some scenarios there are large number of new
>> reads and rarely hit, so it's necessary to disable the refill when read
>> miss to save space for writes.
>>
>> This patch add a new config called refill_on_miss_disabled which is
>> not set
>> by default. Bcache user can set it by sysfs interface and then the bcache
>> device will not refill when read cache miss.
>>
>> Signed-off-by: Guoju Fang <fangguoju@gmail.com>
>> ---
>>   drivers/md/bcache/bcache.h  | 1 +
>>   drivers/md/bcache/request.c | 2 ++
>>   drivers/md/bcache/super.c   | 1 +
>>   drivers/md/bcache/sysfs.c   | 5 +++++
>>   4 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
>> index 221e0191b687..3a19ee6de3a7 100644
>> --- a/drivers/md/bcache/bcache.h
>> +++ b/drivers/md/bcache/bcache.h
>> @@ -730,6 +730,7 @@ struct cache_set {
>>       unsigned int        shrinker_disabled:1;
>>       unsigned int        copy_gc_enabled:1;
>>       unsigned int        idle_max_writeback_rate_enabled:1;
>> +    unsigned int        refill_on_miss_disabled:1;
>>     #define BUCKET_HASH_BITS    12
>>       struct hlist_head    bucket_hash[1 << BUCKET_HASH_BITS];
>> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
>> index 7acf024e99f3..4bfa0e0b4b3f 100644
>> --- a/drivers/md/bcache/request.c
>> +++ b/drivers/md/bcache/request.c
>> @@ -378,6 +378,8 @@ static bool check_should_bypass(struct cached_dev
>> *dc, struct bio *bio)
>>            op_is_write(bio_op(bio))))
>>           goto skip;
>>   +    if (c->refill_on_miss_disabled && !op_is_write(bio_op(bio)))
>> +        goto skip;
>>       /*
>>        * If the bio is for read-ahead or background IO, bypass it or
>>        * not depends on the following situations,
>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>> index 2014016f9a60..c1e9bfec1267 100644
>> --- a/drivers/md/bcache/super.c
>> +++ b/drivers/md/bcache/super.c
>> @@ -1862,6 +1862,7 @@ struct cache_set *bch_cache_set_alloc(struct
>> cache_sb *sb)
>>       c->congested_write_threshold_us    = 20000;
>>       c->error_limit    = DEFAULT_IO_ERROR_LIMIT;
>>       c->idle_max_writeback_rate_enabled = 1;
>> +    c->refill_on_miss_disabled = 0;
>>       WARN_ON(test_and_clear_bit(CACHE_SET_IO_DISABLE, &c->flags));
>>         return c;
>> diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
>> index 0dadec5a78f6..178300f401bb 100644
>> --- a/drivers/md/bcache/sysfs.c
>> +++ b/drivers/md/bcache/sysfs.c
>> @@ -144,6 +144,7 @@ rw_attribute(copy_gc_enabled);
>>   rw_attribute(idle_max_writeback_rate);
>>   rw_attribute(gc_after_writeback);
>>   rw_attribute(size);
>> +rw_attribute(refill_on_miss_disabled);
>>     static ssize_t bch_snprint_string_list(char *buf,
>>                          size_t size,
>> @@ -779,6 +780,8 @@ SHOW(__bch_cache_set)
>>       if (attr == &sysfs_bset_tree_stats)
>>           return bch_bset_print_stats(c, buf);
>>   +    sysfs_printf(refill_on_miss_disabled, "%i",
>> c->refill_on_miss_disabled);
>> +
>>       return 0;
>>   }
>>   SHOW_LOCKED(bch_cache_set)
>> @@ -898,6 +901,7 @@ STORE(__bch_cache_set)
>>        * set in next chance.
>>        */
>>       sysfs_strtoul_clamp(gc_after_writeback, c->gc_after_writeback,
>> 0, 1);
>> +    sysfs_strtoul(refill_on_miss_disabled, c->refill_on_miss_disabled);
>>         return size;
>>   }
>> @@ -948,6 +952,7 @@ static struct attribute *bch_cache_set_files[] = {
>>       &sysfs_congested_read_threshold_us,
>>       &sysfs_congested_write_threshold_us,
>>       &sysfs_clear_stats,
>> +    &sysfs_refill_on_miss_disabled,
>>       NULL
>>   };
>>   KTYPE(bch_cache_set);
>>

Hi Hannes,

> Please don't call the attribute refill_on_miss_disabled.
> This kind of double-negation will always lead to issues; please invert
> the meaning and call it 'refill_on_miss'.

The original purpose is to implement a "write-only" or "read-around"
like cache. Such kind of cache model is desired for heavy write
condition, and disable the read-miss-refill may also help to extend the
SSD life time.

Naming is a challenge here. Becuase "write-only" is not true while read
hitting on dirty data on cache device may successfully return, and
"read-around" is also not true due to the same reason.

We do need help for people to suggest a proper mode for such cache mode,
or cache configuration (if we don't want one more cache mode).

Thanks.

Coly Li