Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
On 10/17, Chao Yu wrote:
> On 2018/10/17 10:19, Jaegeuk Kim wrote:
> > On 10/17, Chao Yu wrote:
> >> Hi Jaegeuk, Jens,
> >>
> >> On 2018/10/17 3:23, Jaegeuk Kim wrote:
> >>> Thanks Jens,
> >>>
> >>> On top of the patch killing the dead code, I wrote another one to detect
> >>> the idle time by the internal account logic like below. IMHO, it'd be
> >>> better to decouple f2fs with other layers, if possible.
> >>>
> >>> Thanks,
> >>>
> >>> >From 85daf5190671b3d98ef779bdea77b4a046658708 Mon Sep 17 00:00:00 2001
> >>> From: Jaegeuk Kim
> >>> Date: Tue, 16 Oct 2018 10:20:53 -0700
> >>> Subject: [PATCH] f2fs: account read IOs and use IO counts for is_idle
> >>>
> >>> This patch adds issued read IO counts which is under block layer.
> >>
> >> I think the problem here is, in android environment, there is multiple
> >> kinds of filesystems used in one device (emmc or ufs), so, if f2fs only be
> >> aware of IOs from itself, still we can face IO conflict between f2fs and
> >> other filesystem (ext4 used in system/... partition), it is possible when
> >> we are trigger GC/discard intensively such as in gc_urgent mode, user can
> >> be stuck when loading data from system partition.
> >
> > IMO, we don't need to worry about gc_urgent too much, since it will be set
> > at
>
> For most of the time, it's okay, in android idle time (IIRC, in middle
> night, charging, screen shutdowns for a while) can be broken by user who
> has bad habit that using cell phone in middle night randomly then user
> operation can be stuck due to IO busy storage.
It's only triggered once a day. And, I don't think /system is only giving
IOs at that moment.
>
> > the given android idle time. Other than that, I think it won't trigger GC
> > and
> > discard too frequently which would be able to affect /system operations.
>
> How about considering low-end products? since performance of storage device
> is real bad, handling discard/read IO in /data can affect /system operations.
>
> > The new interface would be nice to have, but give more complexity on
> > backporting
> > work in old kernels and newer as well across the layers.
>
> Yes, that's a problem.
>
> BTW, we just encounter 'GC not work' problem in product, I guess that's due
> to is_idle()'s problem, let's try to confirm the root cause.
>
> Thanks,
>
> >
> >>
> >> So I guess the better way to avoid this is to export IO busy status in
> >> block layer to filesystem, in f2fs, we can provider effective service in
> >> background thread at real idle time, and will not impact user.
> >>
> >> Any thoughts?
> >>
> >> Thanks,
> >>
> >>>
> >>> Signed-off-by: Jaegeuk Kim
> >>> ---
> >>> fs/f2fs/data.c | 24 ++--
> >>> fs/f2fs/debug.c | 7 ++-
> >>> fs/f2fs/f2fs.h | 22 --
> >>> 3 files changed, 44 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 8952f2d610a6..5fdc8d751f19 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -52,6 +52,23 @@ static bool __is_cp_guaranteed(struct page *page)
> >>> return false;
> >>> }
> >>>
> >>> +static enum count_type __read_io_type(struct page *page)
> >>> +{
> >>> + struct address_space *mapping = page->mapping;
> >>> +
> >>> + if (mapping) {
> >>> + struct inode *inode = mapping->host;
> >>> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >>> +
> >>> + if (inode->i_ino == F2FS_META_INO(sbi))
> >>> + return F2FS_RD_META;
> >>> +
> >>> + if (inode->i_ino == F2FS_NODE_INO(sbi))
> >>> + return F2FS_RD_NODE;
> >>> + }
> >>> + return F2FS_RD_DATA;
> >>> +}
> >>> +
> >>> /* postprocessing steps for read bios */
> >>> enum bio_post_read_step {
> >>> STEP_INITIAL = 0,
> >>> @@ -82,6 +99,7 @@ static void __read_end_io(struct bio *bio)
> >>> } else {
> >>> SetPageUptodate(page);
> >>> }
> >>> + dec_page_count(F2FS_P_SB(page), __read_io_type(page));
> >>> unlock_page(page);
> >>> }
> >>> if (bio->bi_private)
> >>> @@ -464,8 +482,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> >>>
> >>> __submit_bio(fio->sbi, bio, fio->type);
> >>>
> >>> - if (!is_read_io(fio->op))
> >>> - inc_page_count(fio->sbi, WB_DATA_TYPE(fio->page));
> >>> + inc_page_count(fio->sbi, is_read_io(fio->op) ?
> >>> + __read_io_type(page): WB_DATA_TYPE(fio->page));
> >>> return 0;
> >>> }
> >>>
> >>> @@ -595,6 +613,7 @@ static int f2fs_submit_page_read(struct inode *inode,
> >>> struct page *page,
> >>> }
> >>> ClearPageError(page);
> >>> __submit_bio(F2FS_I_SB(inode), bio, DATA);
> >>> + inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
> >>> return 0;
> >>> }
> >>>
> >>> @@ -1593,6 +1612,7 @@ static int f2fs_mpage_readpages(struct
> >>> address_space *mapping,
> >>> if (bio_add_page(bio, page, blocksize, 0) < blocksize)
> >>> goto submit_and_realloc;
> >>>
>
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
On 2018/10/17 10:19, Jaegeuk Kim wrote:
> On 10/17, Chao Yu wrote:
>> Hi Jaegeuk, Jens,
>>
>> On 2018/10/17 3:23, Jaegeuk Kim wrote:
>>> Thanks Jens,
>>>
>>> On top of the patch killing the dead code, I wrote another one to detect
>>> the idle time by the internal account logic like below. IMHO, it'd be
>>> better to decouple f2fs with other layers, if possible.
>>>
>>> Thanks,
>>>
>>> >From 85daf5190671b3d98ef779bdea77b4a046658708 Mon Sep 17 00:00:00 2001
>>> From: Jaegeuk Kim
>>> Date: Tue, 16 Oct 2018 10:20:53 -0700
>>> Subject: [PATCH] f2fs: account read IOs and use IO counts for is_idle
>>>
>>> This patch adds issued read IO counts which is under block layer.
>>
>> I think the problem here is, in android environment, there is multiple
>> kinds of filesystems used in one device (emmc or ufs), so, if f2fs only be
>> aware of IOs from itself, still we can face IO conflict between f2fs and
>> other filesystem (ext4 used in system/... partition), it is possible when
>> we are trigger GC/discard intensively such as in gc_urgent mode, user can
>> be stuck when loading data from system partition.
>
> IMO, we don't need to worry about gc_urgent too much, since it will be set at
For most of the time, it's okay, in android idle time (IIRC, in middle
night, charging, screen shutdowns for a while) can be broken by user who
has bad habit that using cell phone in middle night randomly then user
operation can be stuck due to IO busy storage.
> the given android idle time. Other than that, I think it won't trigger GC and
> discard too frequently which would be able to affect /system operations.
How about considering low-end products? since performance of storage device
is real bad, handling discard/read IO in /data can affect /system operations.
> The new interface would be nice to have, but give more complexity on
> backporting
> work in old kernels and newer as well across the layers.
Yes, that's a problem.
BTW, we just encounter 'GC not work' problem in product, I guess that's due
to is_idle()'s problem, let's try to confirm the root cause.
Thanks,
>
>>
>> So I guess the better way to avoid this is to export IO busy status in
>> block layer to filesystem, in f2fs, we can provider effective service in
>> background thread at real idle time, and will not impact user.
>>
>> Any thoughts?
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jaegeuk Kim
>>> ---
>>> fs/f2fs/data.c | 24 ++--
>>> fs/f2fs/debug.c | 7 ++-
>>> fs/f2fs/f2fs.h | 22 --
>>> 3 files changed, 44 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 8952f2d610a6..5fdc8d751f19 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -52,6 +52,23 @@ static bool __is_cp_guaranteed(struct page *page)
>>> return false;
>>> }
>>>
>>> +static enum count_type __read_io_type(struct page *page)
>>> +{
>>> + struct address_space *mapping = page->mapping;
>>> +
>>> + if (mapping) {
>>> + struct inode *inode = mapping->host;
>>> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>> +
>>> + if (inode->i_ino == F2FS_META_INO(sbi))
>>> + return F2FS_RD_META;
>>> +
>>> + if (inode->i_ino == F2FS_NODE_INO(sbi))
>>> + return F2FS_RD_NODE;
>>> + }
>>> + return F2FS_RD_DATA;
>>> +}
>>> +
>>> /* postprocessing steps for read bios */
>>> enum bio_post_read_step {
>>> STEP_INITIAL = 0,
>>> @@ -82,6 +99,7 @@ static void __read_end_io(struct bio *bio)
>>> } else {
>>> SetPageUptodate(page);
>>> }
>>> + dec_page_count(F2FS_P_SB(page), __read_io_type(page));
>>> unlock_page(page);
>>> }
>>> if (bio->bi_private)
>>> @@ -464,8 +482,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>>>
>>> __submit_bio(fio->sbi, bio, fio->type);
>>>
>>> - if (!is_read_io(fio->op))
>>> - inc_page_count(fio->sbi, WB_DATA_TYPE(fio->page));
>>> + inc_page_count(fio->sbi, is_read_io(fio->op) ?
>>> + __read_io_type(page): WB_DATA_TYPE(fio->page));
>>> return 0;
>>> }
>>>
>>> @@ -595,6 +613,7 @@ static int f2fs_submit_page_read(struct inode *inode,
>>> struct page *page,
>>> }
>>> ClearPageError(page);
>>> __submit_bio(F2FS_I_SB(inode), bio, DATA);
>>> + inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
>>> return 0;
>>> }
>>>
>>> @@ -1593,6 +1612,7 @@ static int f2fs_mpage_readpages(struct address_space
>>> *mapping,
>>> if (bio_add_page(bio, page, blocksize, 0) < blocksize)
>>> goto submit_and_realloc;
>>>
>>> + inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
>>> ClearPageError(page);
>>> last_block_in_bio = block_nr;
>>> goto next_page;
>>> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
>>> index 026e10f30889..139b4d5c83d5 100644
>>> --- a/fs/f2fs/debug.c
>>> +++ b/fs/f2fs/debug.c
>>>
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
On 10/17, Chao Yu wrote:
> Hi Jaegeuk, Jens,
>
> On 2018/10/17 3:23, Jaegeuk Kim wrote:
> > Thanks Jens,
> >
> > On top of the patch killing the dead code, I wrote another one to detect
> > the idle time by the internal account logic like below. IMHO, it'd be
> > better to decouple f2fs with other layers, if possible.
> >
> > Thanks,
> >
> >>From 85daf5190671b3d98ef779bdea77b4a046658708 Mon Sep 17 00:00:00 2001
> > From: Jaegeuk Kim
> > Date: Tue, 16 Oct 2018 10:20:53 -0700
> > Subject: [PATCH] f2fs: account read IOs and use IO counts for is_idle
> >
> > This patch adds issued read IO counts which is under block layer.
>
> I think the problem here is, in android environment, there is multiple
> kinds of filesystems used in one device (emmc or ufs), so, if f2fs only be
> aware of IOs from itself, still we can face IO conflict between f2fs and
> other filesystem (ext4 used in system/... partition), it is possible when
> we are trigger GC/discard intensively such as in gc_urgent mode, user can
> be stuck when loading data from system partition.
IMO, we don't need to worry about gc_urgent too much, since it will be set at
the given android idle time. Other than that, I think it won't trigger GC and
discard too frequently which would be able to affect /system operations.
The new interface would be nice to have, but give more complexity on backporting
work in old kernels and newer as well across the layers.
>
> So I guess the better way to avoid this is to export IO busy status in
> block layer to filesystem, in f2fs, we can provider effective service in
> background thread at real idle time, and will not impact user.
>
> Any thoughts?
>
> Thanks,
>
> >
> > Signed-off-by: Jaegeuk Kim
> > ---
> > fs/f2fs/data.c | 24 ++--
> > fs/f2fs/debug.c | 7 ++-
> > fs/f2fs/f2fs.h | 22 --
> > 3 files changed, 44 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 8952f2d610a6..5fdc8d751f19 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -52,6 +52,23 @@ static bool __is_cp_guaranteed(struct page *page)
> > return false;
> > }
> >
> > +static enum count_type __read_io_type(struct page *page)
> > +{
> > + struct address_space *mapping = page->mapping;
> > +
> > + if (mapping) {
> > + struct inode *inode = mapping->host;
> > + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > +
> > + if (inode->i_ino == F2FS_META_INO(sbi))
> > + return F2FS_RD_META;
> > +
> > + if (inode->i_ino == F2FS_NODE_INO(sbi))
> > + return F2FS_RD_NODE;
> > + }
> > + return F2FS_RD_DATA;
> > +}
> > +
> > /* postprocessing steps for read bios */
> > enum bio_post_read_step {
> > STEP_INITIAL = 0,
> > @@ -82,6 +99,7 @@ static void __read_end_io(struct bio *bio)
> > } else {
> > SetPageUptodate(page);
> > }
> > + dec_page_count(F2FS_P_SB(page), __read_io_type(page));
> > unlock_page(page);
> > }
> > if (bio->bi_private)
> > @@ -464,8 +482,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> >
> > __submit_bio(fio->sbi, bio, fio->type);
> >
> > - if (!is_read_io(fio->op))
> > - inc_page_count(fio->sbi, WB_DATA_TYPE(fio->page));
> > + inc_page_count(fio->sbi, is_read_io(fio->op) ?
> > + __read_io_type(page): WB_DATA_TYPE(fio->page));
> > return 0;
> > }
> >
> > @@ -595,6 +613,7 @@ static int f2fs_submit_page_read(struct inode *inode,
> > struct page *page,
> > }
> > ClearPageError(page);
> > __submit_bio(F2FS_I_SB(inode), bio, DATA);
> > + inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
> > return 0;
> > }
> >
> > @@ -1593,6 +1612,7 @@ static int f2fs_mpage_readpages(struct address_space
> > *mapping,
> > if (bio_add_page(bio, page, blocksize, 0) < blocksize)
> > goto submit_and_realloc;
> >
> > + inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
> > ClearPageError(page);
> > last_block_in_bio = block_nr;
> > goto next_page;
> > diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> > index 026e10f30889..139b4d5c83d5 100644
> > --- a/fs/f2fs/debug.c
> > +++ b/fs/f2fs/debug.c
> > @@ -55,6 +55,9 @@ static void update_general_status(struct f2fs_sb_info
> > *sbi)
> > si->max_vw_cnt = atomic_read(>max_vw_cnt);
> > si->nr_wb_cp_data = get_pages(sbi, F2FS_WB_CP_DATA);
> > si->nr_wb_data = get_pages(sbi, F2FS_WB_DATA);
> > + si->nr_rd_data = get_pages(sbi, F2FS_RD_DATA);
> > + si->nr_rd_node = get_pages(sbi, F2FS_RD_NODE);
> > + si->nr_rd_meta = get_pages(sbi, F2FS_RD_META);
> > if (SM_I(sbi) && SM_I(sbi)->fcc_info) {
> > si->nr_flushed =
> > atomic_read(_I(sbi)->fcc_info->issued_flush);
> > @@ -371,7 +374,9 @@ static int stat_show(struct seq_file *s, void *v)
> >
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
Hi Jaegeuk, Jens,
On 2018/10/17 3:23, Jaegeuk Kim wrote:
> Thanks Jens,
>
> On top of the patch killing the dead code, I wrote another one to detect
> the idle time by the internal account logic like below. IMHO, it'd be
> better to decouple f2fs with other layers, if possible.
>
> Thanks,
>
>>From 85daf5190671b3d98ef779bdea77b4a046658708 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim
> Date: Tue, 16 Oct 2018 10:20:53 -0700
> Subject: [PATCH] f2fs: account read IOs and use IO counts for is_idle
>
> This patch adds issued read IO counts which is under block layer.
I think the problem here is, in android environment, there is multiple
kinds of filesystems used in one device (emmc or ufs), so, if f2fs only be
aware of IOs from itself, still we can face IO conflict between f2fs and
other filesystem (ext4 used in system/... partition), it is possible when
we are trigger GC/discard intensively such as in gc_urgent mode, user can
be stuck when loading data from system partition.
So I guess the better way to avoid this is to export IO busy status in
block layer to filesystem, in f2fs, we can provider effective service in
background thread at real idle time, and will not impact user.
Any thoughts?
Thanks,
>
> Signed-off-by: Jaegeuk Kim
> ---
> fs/f2fs/data.c | 24 ++--
> fs/f2fs/debug.c | 7 ++-
> fs/f2fs/f2fs.h | 22 --
> 3 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 8952f2d610a6..5fdc8d751f19 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -52,6 +52,23 @@ static bool __is_cp_guaranteed(struct page *page)
> return false;
> }
>
> +static enum count_type __read_io_type(struct page *page)
> +{
> + struct address_space *mapping = page->mapping;
> +
> + if (mapping) {
> + struct inode *inode = mapping->host;
> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> +
> + if (inode->i_ino == F2FS_META_INO(sbi))
> + return F2FS_RD_META;
> +
> + if (inode->i_ino == F2FS_NODE_INO(sbi))
> + return F2FS_RD_NODE;
> + }
> + return F2FS_RD_DATA;
> +}
> +
> /* postprocessing steps for read bios */
> enum bio_post_read_step {
> STEP_INITIAL = 0,
> @@ -82,6 +99,7 @@ static void __read_end_io(struct bio *bio)
> } else {
> SetPageUptodate(page);
> }
> + dec_page_count(F2FS_P_SB(page), __read_io_type(page));
> unlock_page(page);
> }
> if (bio->bi_private)
> @@ -464,8 +482,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>
> __submit_bio(fio->sbi, bio, fio->type);
>
> - if (!is_read_io(fio->op))
> - inc_page_count(fio->sbi, WB_DATA_TYPE(fio->page));
> + inc_page_count(fio->sbi, is_read_io(fio->op) ?
> + __read_io_type(page): WB_DATA_TYPE(fio->page));
> return 0;
> }
>
> @@ -595,6 +613,7 @@ static int f2fs_submit_page_read(struct inode *inode,
> struct page *page,
> }
> ClearPageError(page);
> __submit_bio(F2FS_I_SB(inode), bio, DATA);
> + inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
> return 0;
> }
>
> @@ -1593,6 +1612,7 @@ static int f2fs_mpage_readpages(struct address_space
> *mapping,
> if (bio_add_page(bio, page, blocksize, 0) < blocksize)
> goto submit_and_realloc;
>
> + inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
> ClearPageError(page);
> last_block_in_bio = block_nr;
> goto next_page;
> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> index 026e10f30889..139b4d5c83d5 100644
> --- a/fs/f2fs/debug.c
> +++ b/fs/f2fs/debug.c
> @@ -55,6 +55,9 @@ static void update_general_status(struct f2fs_sb_info *sbi)
> si->max_vw_cnt = atomic_read(>max_vw_cnt);
> si->nr_wb_cp_data = get_pages(sbi, F2FS_WB_CP_DATA);
> si->nr_wb_data = get_pages(sbi, F2FS_WB_DATA);
> + si->nr_rd_data = get_pages(sbi, F2FS_RD_DATA);
> + si->nr_rd_node = get_pages(sbi, F2FS_RD_NODE);
> + si->nr_rd_meta = get_pages(sbi, F2FS_RD_META);
> if (SM_I(sbi) && SM_I(sbi)->fcc_info) {
> si->nr_flushed =
> atomic_read(_I(sbi)->fcc_info->issued_flush);
> @@ -371,7 +374,9 @@ static int stat_show(struct seq_file *s, void *v)
> seq_printf(s, " - Inner Struct Count: tree: %d(%d), node:
> %d\n",
> si->ext_tree, si->zombie_tree, si->ext_node);
> seq_puts(s, "\nBalancing F2FS Async:\n");
> - seq_printf(s, " - IO (CP: %4d, Data: %4d, Flush: (%4d %4d
> %4d), "
> + seq_printf(s, " - IO_R (Data: %4d, Node: %4d, Meta: %4d\n",
> +si->nr_rd_data, si->nr_rd_node, si->nr_rd_meta);
> + seq_printf(s, " - IO_W (CP: %4d, Data: %4d, Flush: (%4d %4d
> %4d), "
>
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
On 2018/10/17 0:20, Jens Axboe wrote:
> On 10/16/18 10:06 AM, Chao Yu wrote:
>> Hi Jens,
>>
>> On 2018-10-16 22:34, Jens Axboe wrote:
>>> This doesn't work on stacked devices, and it doesn't work on
>>> blk-mq devices. The request_list is only used on legacy, which
>>> we don't have much of anymore, and soon won't have any of.
>>>
>>> Kill the check.
>>
>> In order to avoid conflicting with user IO, GC and Discard thread try to be
>> aware of IO status of device by is_idle(), if previous implementation doesn't
>> work, do we have any other existing method to get such status? Or do you have
>> any suggestion on this?
>
> This particular patch just kills code that won't yield any useful
> information on stacked devices or blk-mq. As those are the only
> ones that will exist shortly, it's dead.
Yes, anyway, I'm okay with that change, please add:
Reviewed-by: Chao Yu
>
> For blk-mq, you could do a busy tag iteration. Something like this.
> That should be done separately though, in essence the existing code
> is dead/useless.
Thanks for given code below, let's discuss this on Jaegeuk's patch. :)
Thanks,
>
>
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 4254e74c1446..823f04209e8c 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -403,6 +403,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q,
> busy_iter_fn *fn,
> }
> blk_queue_exit(q);
> }
> +EXPORT_SYMBOL_GPL(blk_mq_queue_tag_busy_iter);
>
> static int bt_alloc(struct sbitmap_queue *bt, unsigned int depth,
> bool round_robin, int node)
> diff --git a/block/blk-mq-tag.h b/block/blk-mq-tag.h
> index 61deab0b5a5a..5af7ff94b400 100644
> --- a/block/blk-mq-tag.h
> +++ b/block/blk-mq-tag.h
> @@ -33,8 +33,6 @@ extern int blk_mq_tag_update_depth(struct blk_mq_hw_ctx
> *hctx,
> struct blk_mq_tags **tags,
> unsigned int depth, bool can_grow);
> extern void blk_mq_tag_wakeup_all(struct blk_mq_tags *tags, bool);
> -void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn,
> - void *priv);
>
> static inline struct sbq_wait_state *bt_wait_ptr(struct sbitmap_queue *bt,
>struct blk_mq_hw_ctx *hctx)
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 58778992eca4..c3a2a7fe3f88 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1347,18 +1347,6 @@ static inline void f2fs_update_time(struct
> f2fs_sb_info *sbi, int type)
> sbi->last_time[type] = jiffies;
> }
>
> -static inline bool f2fs_time_over(struct f2fs_sb_info *sbi, int type)
> -{
> - unsigned long interval = sbi->interval_time[type] * HZ;
> -
> - return time_after(jiffies, sbi->last_time[type] + interval);
> -}
> -
> -static inline bool is_idle(struct f2fs_sb_info *sbi)
> -{
> - return f2fs_time_over(sbi, REQ_TIME);
> -}
> -
> /*
> * Inline functions
> */
> @@ -2948,6 +2936,7 @@ void f2fs_destroy_segment_manager_caches(void);
> int f2fs_rw_hint_to_seg_type(enum rw_hint hint);
> enum rw_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi,
> enum page_type type, enum temp_type temp);
> +bool f2fs_is_idle(struct f2fs_sb_info *sbi);
>
> /*
> * checkpoint.c
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 5c8d00422237..fb4152527cf1 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -83,7 +83,7 @@ static int gc_thread_func(void *data)
> if (!mutex_trylock(>gc_mutex))
> goto next;
>
> - if (!is_idle(sbi)) {
> + if (!f2fs_is_idle(sbi)) {
> increase_sleep_time(gc_th, _ms);
> mutex_unlock(>gc_mutex);
> goto next;
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 30779aaa9dba..a02c5ecfb2e4 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -11,7 +11,7 @@
> #include
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -33,6 +33,37 @@ static struct kmem_cache *discard_cmd_slab;
> static struct kmem_cache *sit_entry_set_slab;
> static struct kmem_cache *inmem_entry_slab;
>
> +static bool f2fs_time_over(struct f2fs_sb_info *sbi, int type)
> +{
> + unsigned long interval = sbi->interval_time[type] * HZ;
> +
> + return time_after(jiffies, sbi->last_time[type] + interval);
> +}
> +
> +static void is_idle_fn(struct blk_mq_hw_ctx *hctx, struct request *req,
> +void *data, bool reserved)
> +{
> + unsigned int *cnt = data;
> +
> + (*cnt)++;
> +}
> +
> +bool f2fs_is_idle(struct f2fs_sb_info *sbi)
> +{
> + struct block_device *bdev = sbi->sb->s_bdev;
> + struct request_queue *q = bdev_get_queue(bdev);
> +
> + if (q->mq_ops) {
> + unsigned int cnt = 0;
> +
> + blk_mq_queue_tag_busy_iter(q, is_idle_fn, );
> + if (cnt)
> + return
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
On 10/16/18 1:31 PM, Jaegeuk Kim wrote: > On 10/16, Jens Axboe wrote: >> On 10/16/18 1:23 PM, Jaegeuk Kim wrote: >>> Thanks Jens, >>> >>> On top of the patch killing the dead code, I wrote another one to detect >>> the idle time by the internal account logic like below. IMHO, it'd be >>> better to decouple f2fs with other layers, if possible. >> >> I agree, that's what got us into trouble in the first place. Can I add >> your acked-by or review-by to the other patch, then? > > I queued the patch in f2fs for -next with SOB, so I guess block for -next > should > be fine to assume this patch is in. Can I? Yes, that's totally fine as well, thanks. -- Jens Axboe ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
On 10/16, Jens Axboe wrote: > On 10/16/18 1:23 PM, Jaegeuk Kim wrote: > > Thanks Jens, > > > > On top of the patch killing the dead code, I wrote another one to detect > > the idle time by the internal account logic like below. IMHO, it'd be > > better to decouple f2fs with other layers, if possible. > > I agree, that's what got us into trouble in the first place. Can I add > your acked-by or review-by to the other patch, then? I queued the patch in f2fs for -next with SOB, so I guess block for -next should be fine to assume this patch is in. Can I? > > -- > Jens Axboe ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
On 10/16/18 1:23 PM, Jaegeuk Kim wrote: > Thanks Jens, > > On top of the patch killing the dead code, I wrote another one to detect > the idle time by the internal account logic like below. IMHO, it'd be > better to decouple f2fs with other layers, if possible. I agree, that's what got us into trouble in the first place. Can I add your acked-by or review-by to the other patch, then? -- Jens Axboe ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
Thanks Jens,
On top of the patch killing the dead code, I wrote another one to detect
the idle time by the internal account logic like below. IMHO, it'd be
better to decouple f2fs with other layers, if possible.
Thanks,
>From 85daf5190671b3d98ef779bdea77b4a046658708 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim
Date: Tue, 16 Oct 2018 10:20:53 -0700
Subject: [PATCH] f2fs: account read IOs and use IO counts for is_idle
This patch adds issued read IO counts which is under block layer.
Signed-off-by: Jaegeuk Kim
---
fs/f2fs/data.c | 24 ++--
fs/f2fs/debug.c | 7 ++-
fs/f2fs/f2fs.h | 22 --
3 files changed, 44 insertions(+), 9 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 8952f2d610a6..5fdc8d751f19 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -52,6 +52,23 @@ static bool __is_cp_guaranteed(struct page *page)
return false;
}
+static enum count_type __read_io_type(struct page *page)
+{
+ struct address_space *mapping = page->mapping;
+
+ if (mapping) {
+ struct inode *inode = mapping->host;
+ struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+
+ if (inode->i_ino == F2FS_META_INO(sbi))
+ return F2FS_RD_META;
+
+ if (inode->i_ino == F2FS_NODE_INO(sbi))
+ return F2FS_RD_NODE;
+ }
+ return F2FS_RD_DATA;
+}
+
/* postprocessing steps for read bios */
enum bio_post_read_step {
STEP_INITIAL = 0,
@@ -82,6 +99,7 @@ static void __read_end_io(struct bio *bio)
} else {
SetPageUptodate(page);
}
+ dec_page_count(F2FS_P_SB(page), __read_io_type(page));
unlock_page(page);
}
if (bio->bi_private)
@@ -464,8 +482,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
__submit_bio(fio->sbi, bio, fio->type);
- if (!is_read_io(fio->op))
- inc_page_count(fio->sbi, WB_DATA_TYPE(fio->page));
+ inc_page_count(fio->sbi, is_read_io(fio->op) ?
+ __read_io_type(page): WB_DATA_TYPE(fio->page));
return 0;
}
@@ -595,6 +613,7 @@ static int f2fs_submit_page_read(struct inode *inode,
struct page *page,
}
ClearPageError(page);
__submit_bio(F2FS_I_SB(inode), bio, DATA);
+ inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
return 0;
}
@@ -1593,6 +1612,7 @@ static int f2fs_mpage_readpages(struct address_space
*mapping,
if (bio_add_page(bio, page, blocksize, 0) < blocksize)
goto submit_and_realloc;
+ inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
ClearPageError(page);
last_block_in_bio = block_nr;
goto next_page;
diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
index 026e10f30889..139b4d5c83d5 100644
--- a/fs/f2fs/debug.c
+++ b/fs/f2fs/debug.c
@@ -55,6 +55,9 @@ static void update_general_status(struct f2fs_sb_info *sbi)
si->max_vw_cnt = atomic_read(>max_vw_cnt);
si->nr_wb_cp_data = get_pages(sbi, F2FS_WB_CP_DATA);
si->nr_wb_data = get_pages(sbi, F2FS_WB_DATA);
+ si->nr_rd_data = get_pages(sbi, F2FS_RD_DATA);
+ si->nr_rd_node = get_pages(sbi, F2FS_RD_NODE);
+ si->nr_rd_meta = get_pages(sbi, F2FS_RD_META);
if (SM_I(sbi) && SM_I(sbi)->fcc_info) {
si->nr_flushed =
atomic_read(_I(sbi)->fcc_info->issued_flush);
@@ -371,7 +374,9 @@ static int stat_show(struct seq_file *s, void *v)
seq_printf(s, " - Inner Struct Count: tree: %d(%d), node:
%d\n",
si->ext_tree, si->zombie_tree, si->ext_node);
seq_puts(s, "\nBalancing F2FS Async:\n");
- seq_printf(s, " - IO (CP: %4d, Data: %4d, Flush: (%4d %4d
%4d), "
+ seq_printf(s, " - IO_R (Data: %4d, Node: %4d, Meta: %4d\n",
+ si->nr_rd_data, si->nr_rd_node, si->nr_rd_meta);
+ seq_printf(s, " - IO_W (CP: %4d, Data: %4d, Flush: (%4d %4d
%4d), "
"Discard: (%4d %4d)) cmd: %4d undiscard:%4u\n",
si->nr_wb_cp_data, si->nr_wb_data,
si->nr_flushing, si->nr_flushed,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 156e6cd2c812..5c80eca194b5 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -950,6 +950,9 @@ enum count_type {
F2FS_DIRTY_IMETA,
F2FS_WB_CP_DATA,
F2FS_WB_DATA,
+ F2FS_RD_DATA,
+ F2FS_RD_NODE,
+ F2FS_RD_META,
NR_COUNT_TYPE,
};
@@ -1392,11 +1395,6 @@ static inline unsigned int f2fs_time_to_wait(struct
f2fs_sb_info *sbi,
return wait_ms;
}
-static inline bool is_idle(struct f2fs_sb_info *sbi, int type)
-{
- return f2fs_time_over(sbi, type);
-}
-
/*
* Inline functions
*/
@@ -1787,7 +1785,9 @@ static inline void
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
On 10/16/18 10:06 AM, Chao Yu wrote:
> Hi Jens,
>
> On 2018-10-16 22:34, Jens Axboe wrote:
>> This doesn't work on stacked devices, and it doesn't work on
>> blk-mq devices. The request_list is only used on legacy, which
>> we don't have much of anymore, and soon won't have any of.
>>
>> Kill the check.
>
> In order to avoid conflicting with user IO, GC and Discard thread try to be
> aware of IO status of device by is_idle(), if previous implementation doesn't
> work, do we have any other existing method to get such status? Or do you have
> any suggestion on this?
This particular patch just kills code that won't yield any useful
information on stacked devices or blk-mq. As those are the only
ones that will exist shortly, it's dead.
For blk-mq, you could do a busy tag iteration. Something like this.
That should be done separately though, in essence the existing code
is dead/useless.
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 4254e74c1446..823f04209e8c 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -403,6 +403,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q,
busy_iter_fn *fn,
}
blk_queue_exit(q);
}
+EXPORT_SYMBOL_GPL(blk_mq_queue_tag_busy_iter);
static int bt_alloc(struct sbitmap_queue *bt, unsigned int depth,
bool round_robin, int node)
diff --git a/block/blk-mq-tag.h b/block/blk-mq-tag.h
index 61deab0b5a5a..5af7ff94b400 100644
--- a/block/blk-mq-tag.h
+++ b/block/blk-mq-tag.h
@@ -33,8 +33,6 @@ extern int blk_mq_tag_update_depth(struct blk_mq_hw_ctx *hctx,
struct blk_mq_tags **tags,
unsigned int depth, bool can_grow);
extern void blk_mq_tag_wakeup_all(struct blk_mq_tags *tags, bool);
-void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn,
- void *priv);
static inline struct sbq_wait_state *bt_wait_ptr(struct sbitmap_queue *bt,
struct blk_mq_hw_ctx *hctx)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 58778992eca4..c3a2a7fe3f88 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1347,18 +1347,6 @@ static inline void f2fs_update_time(struct f2fs_sb_info
*sbi, int type)
sbi->last_time[type] = jiffies;
}
-static inline bool f2fs_time_over(struct f2fs_sb_info *sbi, int type)
-{
- unsigned long interval = sbi->interval_time[type] * HZ;
-
- return time_after(jiffies, sbi->last_time[type] + interval);
-}
-
-static inline bool is_idle(struct f2fs_sb_info *sbi)
-{
- return f2fs_time_over(sbi, REQ_TIME);
-}
-
/*
* Inline functions
*/
@@ -2948,6 +2936,7 @@ void f2fs_destroy_segment_manager_caches(void);
int f2fs_rw_hint_to_seg_type(enum rw_hint hint);
enum rw_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi,
enum page_type type, enum temp_type temp);
+bool f2fs_is_idle(struct f2fs_sb_info *sbi);
/*
* checkpoint.c
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 5c8d00422237..fb4152527cf1 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -83,7 +83,7 @@ static int gc_thread_func(void *data)
if (!mutex_trylock(>gc_mutex))
goto next;
- if (!is_idle(sbi)) {
+ if (!f2fs_is_idle(sbi)) {
increase_sleep_time(gc_th, _ms);
mutex_unlock(>gc_mutex);
goto next;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 30779aaa9dba..a02c5ecfb2e4 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -11,7 +11,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
@@ -33,6 +33,37 @@ static struct kmem_cache *discard_cmd_slab;
static struct kmem_cache *sit_entry_set_slab;
static struct kmem_cache *inmem_entry_slab;
+static bool f2fs_time_over(struct f2fs_sb_info *sbi, int type)
+{
+ unsigned long interval = sbi->interval_time[type] * HZ;
+
+ return time_after(jiffies, sbi->last_time[type] + interval);
+}
+
+static void is_idle_fn(struct blk_mq_hw_ctx *hctx, struct request *req,
+ void *data, bool reserved)
+{
+ unsigned int *cnt = data;
+
+ (*cnt)++;
+}
+
+bool f2fs_is_idle(struct f2fs_sb_info *sbi)
+{
+ struct block_device *bdev = sbi->sb->s_bdev;
+ struct request_queue *q = bdev_get_queue(bdev);
+
+ if (q->mq_ops) {
+ unsigned int cnt = 0;
+
+ blk_mq_queue_tag_busy_iter(q, is_idle_fn, );
+ if (cnt)
+ return false;
+ }
+
+ return f2fs_time_over(sbi, REQ_TIME);
+}
+
static unsigned long __reverse_ulong(unsigned char *str)
{
unsigned long tmp = 0;
@@ -511,7 +542,7 @@ void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi)
else
f2fs_build_free_nids(sbi, false, false);
- if (!is_idle(sbi) &&
+ if (!f2fs_is_idle(sbi) &&
Re: [f2fs-dev] [PATCH] f2fs: remove request_list check in is_idle()
Hi Jens,
On 2018-10-16 22:34, Jens Axboe wrote:
> This doesn't work on stacked devices, and it doesn't work on
> blk-mq devices. The request_list is only used on legacy, which
> we don't have much of anymore, and soon won't have any of.
>
> Kill the check.
In order to avoid conflicting with user IO, GC and Discard thread try to be
aware of IO status of device by is_idle(), if previous implementation doesn't
work, do we have any other existing method to get such status? Or do you have
any suggestion on this?
Thanks,
>
> Cc: Jaegeuk Kim
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Signed-off-by: Jens Axboe
> ---
> fs/f2fs/f2fs.h | 7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index abf925664d9c..58778992eca4 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1356,13 +1356,6 @@ static inline bool f2fs_time_over(struct f2fs_sb_info
> *sbi, int type)
>
> static inline bool is_idle(struct f2fs_sb_info *sbi)
> {
> - struct block_device *bdev = sbi->sb->s_bdev;
> - struct request_queue *q = bdev_get_queue(bdev);
> - struct request_list *rl = >root_rl;
> -
> - if (rl->count[BLK_RW_SYNC] || rl->count[BLK_RW_ASYNC])
> - return false;
> -
> return f2fs_time_over(sbi, REQ_TIME);
> }
>
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
