| Message ID | 20200123170142.98974-15-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bcache patches for Linux v5.6 | expand |
Hi Coly and Jens-- One concern I have with this is that it's going to wear out limited-lifetime SSDs a -lot- faster. Was any thought given to making this a tunable instead of just changing the behavior? Even if we have an anecdote or two that it seems to have increased performance for some workloads, I don't expect it will have increased performance in general and it may even be costly for some workloads (it all comes down to what is more useful in the cache-- somewhat-recently readahead data, or the data that it is displacing). Regards, Mike On Thu, Jan 23, 2020 at 9:03 AM <colyli@suse.de> wrote: > > From: Coly Li <colyli@suse.de> > > In year 2007 high performance SSD was still expensive, in order to > save more space for real workload or meta data, the readahead I/Os > for non-meta data was bypassed and not cached on SSD. > > In now days, SSD price drops a lot and people can find larger size > SSD with more comfortable price. It is unncessary to bypass normal > readahead I/Os to save SSD space for now. > > This patch removes the code which checks REQ_RAHEAD tag of bio in > check_should_bypass(), then all readahead I/Os will be cached on SSD. > > NOTE: this patch still keeps the checking of "REQ_META|REQ_PRIO" in > should_writeback(), because we still want to cache meta data I/Os > even they are asynchronized. > > Cc: stable@vger.kernel.org > Signed-off-by: Coly Li <colyli@suse.de> > Acked-by: Eric Wheeler <bcache@linux.ewheeler.net> > --- > drivers/md/bcache/request.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c > index 73478a91a342..acc07c4f27ae 100644 > --- a/drivers/md/bcache/request.c > +++ b/drivers/md/bcache/request.c > @@ -378,15 +378,6 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio) > op_is_write(bio_op(bio)))) > goto skip; > > - /* > - * Flag for bypass if the IO is for read-ahead or background, > - * unless the read-ahead request is for metadata > - * (eg, for gfs2 or xfs). > - */ > - if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) && > - !(bio->bi_opf & (REQ_META|REQ_PRIO))) > - goto skip; > - > if (bio->bi_iter.bi_sector & (c->sb.block_size - 1) || > bio_sectors(bio) & (c->sb.block_size - 1)) { > pr_debug("skipping unaligned io"); > -- > 2.16.4 >
On 2020/1/24 1:19 上午, Michael Lyle wrote: > Hi Coly and Jens-- > > One concern I have with this is that it's going to wear out > limited-lifetime SSDs a -lot- faster. Was any thought given to making > this a tunable instead of just changing the behavior? Even if we have > an anecdote or two that it seems to have increased performance for > some workloads, I don't expect it will have increased performance in > general and it may even be costly for some workloads (it all comes > down to what is more useful in the cache-- somewhat-recently readahead > data, or the data that it is displacing). Hi Mike, Copied. This is good suggestion, I will do it after I back from Lunar New Year vacation, and submit it with other tested patches in following v5.6-rc versions. Thanks. Coly Li [snipped]
On 1/23/20 10:27 AM, Coly Li wrote: > On 2020/1/24 1:19 上午, Michael Lyle wrote: >> Hi Coly and Jens-- >> >> One concern I have with this is that it's going to wear out >> limited-lifetime SSDs a -lot- faster. Was any thought given to making >> this a tunable instead of just changing the behavior? Even if we have >> an anecdote or two that it seems to have increased performance for >> some workloads, I don't expect it will have increased performance in >> general and it may even be costly for some workloads (it all comes >> down to what is more useful in the cache-- somewhat-recently readahead >> data, or the data that it is displacing). > > Hi Mike, > > Copied. This is good suggestion, I will do it after I back from Lunar > New Year vacation, and submit it with other tested patches in following > v5.6-rc versions. Do you want me to just drop this patch for now from the series?
On 2020/1/24 2:31 上午, Jens Axboe wrote: > On 1/23/20 10:27 AM, Coly Li wrote: >> On 2020/1/24 1:19 上午, Michael Lyle wrote: >>> Hi Coly and Jens-- >>> >>> One concern I have with this is that it's going to wear out >>> limited-lifetime SSDs a -lot- faster. Was any thought given to making >>> this a tunable instead of just changing the behavior? Even if we have >>> an anecdote or two that it seems to have increased performance for >>> some workloads, I don't expect it will have increased performance in >>> general and it may even be costly for some workloads (it all comes >>> down to what is more useful in the cache-- somewhat-recently readahead >>> data, or the data that it is displacing). >> >> Hi Mike, >> >> Copied. This is good suggestion, I will do it after I back from Lunar >> New Year vacation, and submit it with other tested patches in following >> v5.6-rc versions. > > Do you want me to just drop this patch for now from the series? > Hi Jens, OK, please drop this patch from this series. I will re-submit the patch with sysfs interface later with other patches. Thanks.
On 1/23/20 5:49 PM, Coly Li wrote: > On 2020/1/24 2:31 上午, Jens Axboe wrote: >> On 1/23/20 10:27 AM, Coly Li wrote: >>> On 2020/1/24 1:19 上午, Michael Lyle wrote: >>>> Hi Coly and Jens-- >>>> >>>> One concern I have with this is that it's going to wear out >>>> limited-lifetime SSDs a -lot- faster. Was any thought given to making >>>> this a tunable instead of just changing the behavior? Even if we have >>>> an anecdote or two that it seems to have increased performance for >>>> some workloads, I don't expect it will have increased performance in >>>> general and it may even be costly for some workloads (it all comes >>>> down to what is more useful in the cache-- somewhat-recently readahead >>>> data, or the data that it is displacing). >>> >>> Hi Mike, >>> >>> Copied. This is good suggestion, I will do it after I back from Lunar >>> New Year vacation, and submit it with other tested patches in following >>> v5.6-rc versions. >> >> Do you want me to just drop this patch for now from the series? >> > Hi Jens, > > OK, please drop this patch from this series. I will re-submit the patch > with sysfs interface later with other patches. Sounds good, I queued up the rest for 5.6.
Hi Coly--- Thank you for holding the patch. I'm sorry for the late review (I was travelling). (We sure have a lot of settings and a lot of code dealing with them all, which is unfortunate... but workloads / hardware used with bcache are so varied). Mike On Thu, Jan 23, 2020 at 9:28 AM Coly Li <colyli@suse.de> wrote: > > On 2020/1/24 1:19 上午, Michael Lyle wrote: > > Hi Coly and Jens-- > > > > One concern I have with this is that it's going to wear out > > limited-lifetime SSDs a -lot- faster. Was any thought given to making > > this a tunable instead of just changing the behavior? Even if we have > > an anecdote or two that it seems to have increased performance for > > some workloads, I don't expect it will have increased performance in > > general and it may even be costly for some workloads (it all comes > > down to what is more useful in the cache-- somewhat-recently readahead > > data, or the data that it is displacing). > > Hi Mike, > > Copied. This is good suggestion, I will do it after I back from Lunar > New Year vacation, and submit it with other tested patches in following > v5.6-rc versions. > > Thanks. > > Coly Li > > [snipped] > > -- > > Coly Li
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c index 73478a91a342..acc07c4f27ae 100644 --- a/drivers/md/bcache/request.c +++ b/drivers/md/bcache/request.c @@ -378,15 +378,6 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio) op_is_write(bio_op(bio)))) goto skip; - /* - * Flag for bypass if the IO is for read-ahead or background, - * unless the read-ahead request is for metadata - * (eg, for gfs2 or xfs). - */ - if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) && - !(bio->bi_opf & (REQ_META|REQ_PRIO))) - goto skip; - if (bio->bi_iter.bi_sector & (c->sb.block_size - 1) || bio_sectors(bio) & (c->sb.block_size - 1)) { pr_debug("skipping unaligned io");