On Sat, Mar 23, 2024 at 09:04:51AM +1100, Dave Chinner wrote:
> On Fri, Mar 22, 2024 at 09:02:31AM +0100, Pankaj Raghav (Samsung) wrote:
> > > * Bulk filling of pages can take multiple calls. Not filling the entire
> > > @@ -426,7 +484,7 @@ _xfs_buf_map_folios(
> > > {
> > > ASSERT(bp->b_flags & _XBF_FOLIOS);
> > > if (bp->b_folio_count == 1) {
> > > - /* A single page buffer is always mappable */
> > > + /* A single folio buffer is always mappable */
> > > bp->b_addr = folio_address(bp->b_folios[0]);
> > > } else if (flags & XBF_UNMAPPED) {
> > > bp->b_addr = NULL;
> > > @@ -1525,20 +1583,28 @@ xfs_buf_ioapply_map(
> > > int *count,
> > > blk_opf_t op)
> > > {
> > > - int page_index;
> > > - unsigned int total_nr_pages = bp->b_folio_count;
> > > - int nr_pages;
> > > + int folio_index;
> > > + unsigned int total_nr_folios = bp->b_folio_count;
> > > + int nr_folios;
> > > struct bio *bio;
> > > sector_t sector = bp->b_maps[map].bm_bn;
> > > int size;
> > > int offset;
> > >
> > > - /* skip the pages in the buffer before the start offset */
> > > - page_index = 0;
> > > + /*
> > > + * If the start offset if larger than a single page, we need to be
> > > + * careful. We might have a high order folio, in which case the indexing
> > > + * is from the start of the buffer. However, if we have more than one
> > > + * folio single page folio in the buffer, we need to skip the folios in
> > s/folio single page folio/single page folio/
> >
> > > + * the buffer before the start offset.
> > > + */
> > > + folio_index = 0;
> > > offset = *buf_offset;
> > > - while (offset >= PAGE_SIZE) {
> > > - page_index++;
> > > - offset -= PAGE_SIZE;
> > > + if (bp->b_folio_count > 1) {
> > > + while (offset >= PAGE_SIZE) {
> > > + folio_index++;
> > > + offset -= PAGE_SIZE;
> >
> > Can this be:
> > folio_index = offset >> PAGE_SHIFT;
> > offset = offset_in_page(offset);
> >
> > instead of a loop?
>
> It could, but I'm not going to change it or even bother to fix the
> comment as this code goes away later in the patch set.
>
> See "[PATCH 7/9] xfs: walk b_addr for buffer I/O" later in the
> series - once we can rely on bp->b_addr always being set for buffers
> we convert this code to a simpler and more efficient folio based
> iteration that is not reliant on pages or PAGE_SIZE at all.
>
Ah, I missed seeing the whole series before replying to individual patches.
Nvm.
Thanks for the pointer on patch 7, and I agree that it does not make
sense to change this patch as it goes away later.
--
Pankaj
