2023-09-14 15:46:54

by Jan Kara

Subject: Re: 答复: [PATCH] fs-writeback: writeb ack_sb_inodes : Do not increase 'total_wrote' when nothing is written

On Thu 14-09-23 04:12:31, 郭纯海 wrote:
> > On Wed 13-09-23 07:15:01, Chunhai Guo wrote:
> > > From the dump info, there are only two pages as shown below. One is
> > > updated and another is under writeback. Maybe f2fs counts the
> > > writeback page as a dirty one, so get_dirty_pages() got one. As you
> > > said, maybe this is unreasonable.
> > >
> > > Jaegeuk & Chao, what do you think of this?
> > >
> > >
> > > crash_32> files -p 0xE5A44678
> > > INODE NRPAGES
> > > e5a44678 2
> > >
> > > PAGE PHYSICAL MAPPING INDEX CNT FLAGS
> > > e8d0e338 641de000 e5a44810 0 5 a095
> > locked,waiters,uptodate,lru,private,writeback
> > > e8ad59a0 54528000 e5a44810 1 2 2036
> > referenced,uptodate,lru,active,private
> >
> > Indeed, incrementing pages_skipped when there's no dirty page is a bit odd.
> > That being said we could also harden requeue_inode() - in particular we could do
> > there:
> >
> > if (wbc->pages_skipped) {
> > /*
> > * Writeback is not making progress due to locked buffers.
> > * Skip this inode for now. Although having skipped pages
> > * is odd for clean inodes, it can happen for some
> > * filesystems so handle that gracefully.
> > */
> > if (inode->i_state & I_DIRTY_ALL)
> > redirty_tail_locked(inode, wb);
> > else
> > inode_cgwb_move_to_attached(inode, wb);
> > }
> >
> > Does this fix your problem as well?
> >
> > Honza
>
> Thank you for your reply. Did you forget the 'return' statement? Since I encountered this issue on the 4.19 kernel and there is not inode_cgwb_move_to_attached() yet, I replaced it with inode_io_list_del_locked(). So, below is the test patch I am applying. Please have a check. By the way, the test will take some time. I will provide feedback when it is finished. Thanks.

Yeah, I forgot about the return.

> if (wbc->pages_skipped) {
> /*
> * writeback is not making progress due to locked
> * buffers. Skip this inode for now.
> */
> - redirty_tail_locked(inode, wb);
> + if (inode->i_state & I_DIRTY_ALL)
> + redirty_tail_locked(inode, wb);
> + else
> + inode_io_list_del_locked(inode, wb);
> return;
> }

Looks good. Thanks for testing!

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2023-09-15 19:39:31

by Chunhai Guo

Subject: Re: 答复: [PATCH] fs-writeback: writebac k_sb_inodes: Do not increase 'total_wrote' when nothi ng is written

在 2023/9/14 14:58, Jan Kara 写道:
> On Thu 14-09-23 04:12:31, 郭纯海 wrote:
>>> On Wed 13-09-23 07:15:01, Chunhai Guo wrote:
>>>> From the dump info, there are only two pages as shown below. One is
>>>> updated and another is under writeback. Maybe f2fs counts the
>>>> writeback page as a dirty one, so get_dirty_pages() got one. As you
>>>> said, maybe this is unreasonable.
>>>>
>>>> Jaegeuk & Chao, what do you think of this?
>>>>
>>>>
>>>> crash_32> files -p 0xE5A44678
>>>> INODE NRPAGES
>>>> e5a44678 2
>>>>
>>>> PAGE PHYSICAL MAPPING INDEX CNT FLAGS
>>>> e8d0e338 641de000 e5a44810 0 5 a095
>>> locked,waiters,uptodate,lru,private,writeback
>>>> e8ad59a0 54528000 e5a44810 1 2 2036
>>> referenced,uptodate,lru,active,private
>>>
>>> Indeed, incrementing pages_skipped when there's no dirty page is a bit odd.
>>> That being said we could also harden requeue_inode() - in particular we could do
>>> there:
>>>
>>> if (wbc->pages_skipped) {
>>> /*
>>> * Writeback is not making progress due to locked buffers.
>>> * Skip this inode for now. Although having skipped pages
>>> * is odd for clean inodes, it can happen for some
>>> * filesystems so handle that gracefully.
>>> */
>>> if (inode->i_state & I_DIRTY_ALL)
>>> redirty_tail_locked(inode, wb);
>>> else
>>> inode_cgwb_move_to_attached(inode, wb);
>>> }
>>>
>>> Does this fix your problem as well?
>>>
>>> Honza
>>
>> Thank you for your reply. Did you forget the 'return' statement? Since I encountered this issue on the 4.19 kernel and there is not inode_cgwb_move_to_attached() yet, I replaced it with inode_io_list_del_locked(). So, below is the test patch I am applying. Please have a check. By the way, the test will take some time. I will provide feedback when it is finished. Thanks.
>
> Yeah, I forgot about the return.

Hi Jan,
The test is finished and this patch can fix this issue, too.
Thanks,
>
>> if (wbc->pages_skipped) {
>> /*
>> * writeback is not making progress due to locked
>> * buffers. Skip this inode for now.
>> */
>> - redirty_tail_locked(inode, wb);
>> + if (inode->i_state & I_DIRTY_ALL)
>> + redirty_tail_locked(inode, wb);
>> + else
>> + inode_io_list_del_locked(inode, wb);
>> return;
>> }
>
> Looks good. Thanks for testing!
>
> Honza

2023-09-16 14:34:08

by Chunhai Guo

Subject: 答复: 答复: [PATCH] fs-writeback: writeba ck_sb_inodes: Do not increase 'total_wrote' w hen nothing is written

> >>> On Wed 13-09-23 07:15:01, Chunhai Guo wrote:
> >>>> From the dump info, there are only two pages as shown below. One
> >>>> is updated and another is under writeback. Maybe f2fs counts the
> >>>> writeback page as a dirty one, so get_dirty_pages() got one. As you
> >>>> said, maybe this is unreasonable.
> >>>>
> >>>> Jaegeuk & Chao, what do you think of this?
> >>>>
> >>>>
> >>>> crash_32> files -p 0xE5A44678
> >>>> INODE NRPAGES
> >>>> e5a44678 2
> >>>>
> >>>> PAGE PHYSICAL MAPPING INDEX CNT FLAGS
> >>>> e8d0e338 641de000 e5a44810 0 5 a095
> >>> locked,waiters,uptodate,lru,private,writeback
> >>>> e8ad59a0 54528000 e5a44810 1 2 2036
> >>> referenced,uptodate,lru,active,private
> >>>
> >>> Indeed, incrementing pages_skipped when there's no dirty page is a bit odd.
> >>> That being said we could also harden requeue_inode() - in particular
> >>> we could do
> >>> there:
> >>>
> >>> if (wbc->pages_skipped) {
> >>> /*
> >>> * Writeback is not making progress due to locked buffers.
> >>> * Skip this inode for now. Although having skipped pages
> >>> * is odd for clean inodes, it can happen for some
> >>> * filesystems so handle that gracefully.
> >>> */
> >>> if (inode->i_state & I_DIRTY_ALL)
> >>> redirty_tail_locked(inode, wb);
> >>> else
> >>> inode_cgwb_move_to_attached(inode, wb);
> >>> }
> >>>
> >>> Does this fix your problem as well?
> >>>
> >>>
> >>> Honza
> >>
> >> Thank you for your reply. Did you forget the 'return' statement? Since I
> encountered this issue on the 4.19 kernel and there is not
> inode_cgwb_move_to_attached() yet, I replaced it with
> inode_io_list_del_locked(). So, below is the test patch I am applying. Please
> have a check. By the way, the test will take some time. I will provide feedback
> when it is finished. Thanks.
> >
> > Yeah, I forgot about the return.
>
> Hi Jan,
> The test is finished and this patch can fix this issue, too.
> Thanks,

Hi Jan,
I have send the patch as you suggested.
Thanks,

> >> if (wbc->pages_skipped) {
> >> /*
> >> * writeback is not making progress due to locked
> >> * buffers. Skip this inode for now.
> >> */
> >> - redirty_tail_locked(inode, wb);
> >> + if (inode->i_state & I_DIRTY_ALL)
> >> + redirty_tail_locked(inode, wb);
> >> + else
> >> + inode_io_list_del_locked(inode, wb);
> >> return;
> >> }
> >
> > Looks good. Thanks for testing!
> >
> > Honza