Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Usama Arif <usama.arif@linux.dev>
To: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v2 2/6] ntfs: Remove use of __folio_index in handle_bounds_compressed_page()
Date: Tue, 9 Jun 2026 16:55:03 +0100	[thread overview]
Message-ID: <fda62f90-2c8c-48ec-88ae-310bb5adaa37@linux.dev> (raw)
In-Reply-To: <aigkhkcXuQx7yfPu@casper.infradead.org>



On 09/06/2026 15:34, Matthew Wilcox wrote:
> [Editing your response to patch 3 into here]
> 
> On Tue, Jun 09, 2026 at 06:17:23AM -0700, Usama Arif wrote:
>> On Mon,  8 Jun 2026 22:06:12 +0100 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:
>>
>>> Nobody is supposed to use page->__folio_index.  Use page_offset()
>>> instead, and simplify by working exclusively in loff_t instead of mixing
>>> up loff_t and pgoff_t.
>>>
>>> Signed-off-by: Mattinnhew Wilcox (Oracle) <willy@infradead.org>
>>> ---
>>>  fs/ntfs/compress.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/ntfs/compress.c b/fs/ntfs/compress.c
>>> index c904858dff3d..b279f38636d6 100644
>>> --- a/fs/ntfs/compress.c
>>> +++ b/fs/ntfs/compress.c
>>> @@ -105,13 +105,14 @@ void free_compression_buffers(void)
>>>  static inline void handle_bounds_compressed_page(struct page *page,
>>>  		const loff_t i_size, const s64 initialized_size)
>>>  {
>>> -	if ((page->__folio_index >= (initialized_size >> PAGE_SHIFT)) &&
>>> -			(initialized_size < i_size)) {
>>> +	loff_t pos = page_offset(page);
>>> +
>>> +	if ((pos >= initialized_size) && (initialized_size < i_size)) {
>>>  		u8 *kp = page_address(page);
>>>  		unsigned int kp_ofs;
>>>  
>>>  		ntfs_debug("Zeroing page region outside initialized size.");
>>> -		if (((s64)page->__folio_index << PAGE_SHIFT) >= initialized_size) {
>>> +		if (pos >= initialized_size) {
>>
>> Is the else branch over here unreacheable as the outer if statement
>> already had if ((pos >= initialized_size) &&...
> 
> Are you saying my transformation is non-equivalent (and I have
> introduced a bug), or are you noting that this is a pre-existing bug?
> 
> If the latter, is the error that the else branch exists, or did the
> author intend to write a different test?

The transformation is non-equivalent.

The original code's outer and inner tests are not the same test.
outer: page->__folio_index >= (initialized_size >> PAGE_SHIFT)
inner: page->__folio_index << PAGE_SHIFT

These differ when initialized_size is not page aligned, because the
outer right-hand side floors and the inner left-hand side doesn't.

After your patch, both the outer test and inner test is:
pos >= initialized_size

I think the fix to make it equivaltent is to relax the outer test
so that it includes PAGE_SIZE (maybe?). i.e. make your outer if
statement to be the below to make it equivalent?

if ((pos + PAGE_SIZE > initialized_size) && (initialized_size < i_size)) {

And then the else branch in your code should hopefully make sense.


  reply	other threads:[~2026-06-09 15:55 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-08 21:06 [PATCH v2 0/6] Remove __folio_index again Matthew Wilcox (Oracle)
2026-06-08 21:06 ` [PATCH v2 1/6] ntfs: Inline zero_partial_compressed_page() Matthew Wilcox (Oracle)
2026-06-09  0:18   ` Hyunchul Lee
2026-06-08 21:06 ` [PATCH v2 2/6] ntfs: Remove use of __folio_index in handle_bounds_compressed_page() Matthew Wilcox (Oracle)
2026-06-09  0:19   ` Hyunchul Lee
2026-06-09 13:17   ` Usama Arif
2026-06-09 14:34     ` Matthew Wilcox
2026-06-09 15:55       ` Usama Arif [this message]
2026-06-08 21:06 ` [PATCH v2 3/6] ntfs: Use zero_user_segment() " Matthew Wilcox (Oracle)
2026-06-09  0:20   ` Hyunchul Lee
2026-06-09 13:07   ` Usama Arif
2026-06-08 21:06 ` [PATCH v2 4/6] ntfs: Remove references to page->__folio_index Matthew Wilcox (Oracle)
2026-06-09  0:22   ` Hyunchul Lee
2026-06-08 21:06 ` [PATCH v2 5/6] show_page_info: Remove printing of page index Matthew Wilcox (Oracle)
2026-06-09 11:59   ` Ye Liu
2026-06-09 18:44   ` David Hildenbrand (Arm)
2026-06-08 21:06 ` [PATCH v2 6/6] mm: Remove __folio_index Matthew Wilcox (Oracle)
2026-06-09 18:43   ` David Hildenbrand (Arm)
2026-06-09  5:06 ` [PATCH v2 0/6] Remove __folio_index again Namjae Jeon

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=fda62f90-2c8c-48ec-88ae-310bb5adaa37@linux.dev \
    --to=usama.arif@linux.dev \
    --cc=akpm@linux-foundation.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=willy@infradead.org \
    /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