From: zhangjun <openzhangj@gmail.com>
To: Dave Chinner <david@fromorbit.com>, Richard Weinberger <richard@nod.at>
Cc: Artem Bityutskiy <dedekind1@gmail.com>,
Adrian Hunter <adrian.hunter@intel.com>,
linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
kirill.shutemov@linux.intel.com, hch@lst.de,
linux-fsdevel@vger.kernel.org
Subject: Re: ubifs: fix page_count in ->ubifs_migrate_page()
Date: Fri, 14 Dec 2018 14:15:18 +0800 [thread overview]
Message-ID: <e477c69a-082b-586b-a45a-50dee2b530ca@gmail.com> (raw)
In-Reply-To: <20181213225736.GF29416@dastard>
On 2018/12/14 上午6:57, Dave Chinner wrote:
> On Thu, Dec 13, 2018 at 03:23:37PM +0100, Richard Weinberger wrote:
>> Hello zhangjun,
>>
>> thanks a lot for bringing this up!
>>
>> Am Mittwoch, 12. Dezember 2018, 15:13:57 CET schrieb zhangjun:
>>> Because the PagePrivate() in UBIFS is different meanings,
>>> alloc_cma() will fail when one dirty page cache located in
>>> the type of MIGRATE_CMA
>>>
>>> If not adjust the 'extra_count' for dirty page,
>>> ubifs_migrate_page() -> migrate_page_move_mapping() will
>>> always return -EAGAIN for:
>>> expected_count += page_has_private(page)
>>> This causes the migration to fail until the page cache is cleaned
>>>
>>> In general, PagePrivate() indicates that buff_head is already bound
>>> to this page, and at the same time page_count() will also increase.
>
> That's an invalid assumption.
>
> We should not be trying to infer what PagePrivate() means in code
> that has no business using looking at it i.e. page->private is private
> information for the owner of the page, and it's life cycle and
> intent are unknown to anyone other than the page owner.
>
> e.g. on XFS, a page cache page's page->private /might/ contain a
> struct iomap_page, or it might be NULL. Assigning a struct
> iomap_page to the page does not change the reference count on the
> page. IOWs, the page needs to be handled exactly the same
> way by external code regardless of whether there is somethign
> attached to page->private or not.
>
> Hence it looks to me like the migration code is making invalid
> assumptions about PagePrivate inferring reference counts and so the
> migration code needs to be fixed. Requiring filesystems to work
> around invalid assumptions in the migration code is a sure recipe
> for problems with random filesystems using page->private for their
> own internal purposes....
>
> Cheers,
>
> Dave.
I agree with your main point of view, but for the buffer_head based file
system this assumption is no problem,
and the parameters and comments from the migrate_page_move_mapping()
function:
* 3 for pages with a mapping and PagePrivate/PagePrivate2 set.
This assumption has been explained.
Or to accurately say that the migrate system does not currently have a
generic function for this case.
Since you call the function implemented for buffer_head, you should
follow its rules.
next prev parent reply other threads:[~2018-12-14 6:15 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-12 14:13 ubifs: fix page_count in ->ubifs_migrate_page() zhangjun
2018-12-13 14:23 ` Richard Weinberger
2018-12-13 15:14 ` zhangjun
2018-12-13 22:57 ` Dave Chinner
2018-12-14 6:15 ` zhangjun [this message]
2018-12-14 9:12 ` Gao Xiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e477c69a-082b-586b-a45a-50dee2b530ca@gmail.com \
--to=openzhangj@gmail.com \
--cc=adrian.hunter@intel.com \
--cc=david@fromorbit.com \
--cc=dedekind1@gmail.com \
--cc=hch@lst.de \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=richard@nod.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).