Re: [PATCH v9 11/15] filemap: Add hwpoison handling to filemap_read()

On Thu, Aug 13, 2026 at 11:55:44AM +0100, Pedro Falcato wrote:
> > +static size_t adjust_range_hwpoison(const struct folio *folio, size_t offset,
> > +		size_t bytes)
> > +{
> > +	const struct page *page = folio_page(folio, offset / PAGE_SIZE);
> > +	size_t safe_bytes;
> 						    offset >> PAGE_SHIFT would probably be more idiomatic

$ git grep '/ PAGE_SIZE' mm fs |wc -l
116
$ git grep '>> PAGE_SHIFT' mm fs |wc -l
656

it seems relatively common.  Personally I find / easier to understand
because I sometimes get confused between left and right shifts.  I
don't think I've ever been confused between multiply and divide ;-)

> > +	/* Safe to read the remaining bytes in this page. */
> > +	safe_bytes = PAGE_SIZE - (offset % PAGE_SIZE);
> 			similarly, offset_in_page(offset)

yes, that's easier to read.

> (yes, no codegen difference in both cases, but there could be a difference
> if the ARM dynamic page size stuff ever moves forward)

I rather hope it doesn't ... but I do hope it manages to strength-reduce
if it does.

> > +	page++;
> > +
> > +	/* Check each remaining page as long as we are not done yet. */
> > +	for (; safe_bytes < bytes; safe_bytes += PAGE_SIZE, page++)
> > +		if (is_ref_page_hwpoison(folio, page))
> > +			break;
> > +
> > +	return min(safe_bytes, bytes);
> > +}
> 
> Wouldn't this whole logic fit better in copy_folio_to_iter? shmem for
> instance also handrolls its own hwpoison read_iter logic with
> per-page-copies, etc; not amazing.

I did wonder wbout doing that.  We don't currently have the folio in
copy_page_to_iter(), and copy_folio_to_iter() decays to
copy_page_to_iter() immediately.  So it'd be a larger change.

> >  
> > -			copied = copy_folio_to_iter(folio, offset, bytes, iter);
> > +			copied = adjust_range_hwpoison(folio, offset, bytes);
> > +			if (copied < bytes)
> > +				error = -EIO;
> 
> I don't love reusing copied here, but I guess there isn't much better
> (in order to detect hwpoison-derived truncation)

I thought this was neater than using a separate variable.