Linux-Block Archive mirror
 help / color / mirror / Atom feed
* [PATCH V1] lib/scatterlist: Fix to calculate the last_pg properly
@ 2023-01-11 10:10 Yishai Hadas
  2023-01-11 10:22 ` Chaitanya Kulkarni
  2023-01-16 16:10 ` Jason Gunthorpe
  0 siblings, 2 replies; 3+ messages in thread
From: Yishai Hadas @ 2023-01-11 10:10 UTC (permalink / raw
  To: linux-kernel, linux-block, linux-mm, jgg, axboe, logang, akpm,
	willy
  Cc: hch, alex.williamson, yishaih, leonro, maorg

The last_pg is wrong, it is actually the first page of the last
scatterlist element. To get the last page of the last scatterlist
element we have to add prv->length. So it is checking mergability
against the wrong page, Further, a SG element is not guaranteed to end
on a page boundary, so we have to check the sub page location also for
merge eligibility.

Fix the above by checking physical contiguity based on PFNs, compute the
actual last page and then call pages_are_mergable().

Fixes: 1567b49d1a40 ("lib/scatterlist: add check when merging zone device pages")
Reported-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
Changelog:
v1:
 * Rename 'paddr' to 'next_pfn' as it's actually.
 * Move to use page_to_phys() to clarify the code.
 * Fix to use (next_pfn - 1) instead of (paddr - PAGE_SIZE).
v0:
https://lore.kernel.org/lkml/Y7zyyTxdoJulq7OD@casper.infradead.org/T/

 lib/scatterlist.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index f72aa50c6654..8d7519a8f308 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -470,22 +470,27 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
 		return -EOPNOTSUPP;
 
 	if (sgt_append->prv) {
+		unsigned long next_pfn = (page_to_phys(sg_page(sgt_append->prv)) +
+			sgt_append->prv->offset + sgt_append->prv->length) / PAGE_SIZE;
+
 		if (WARN_ON(offset))
 			return -EINVAL;
 
 		/* Merge contiguous pages into the last SG */
 		prv_len = sgt_append->prv->length;
-		last_pg = sg_page(sgt_append->prv);
-		while (n_pages && pages_are_mergeable(pages[0], last_pg)) {
-			if (sgt_append->prv->length + PAGE_SIZE > max_segment)
-				break;
-			sgt_append->prv->length += PAGE_SIZE;
-			last_pg = pages[0];
-			pages++;
-			n_pages--;
+		if (page_to_pfn(pages[0]) == next_pfn) {
+			last_pg = pfn_to_page(next_pfn - 1);
+			while (n_pages && pages_are_mergeable(pages[0], last_pg)) {
+				if (sgt_append->prv->length + PAGE_SIZE > max_segment)
+					break;
+				sgt_append->prv->length += PAGE_SIZE;
+				last_pg = pages[0];
+				pages++;
+				n_pages--;
+			}
+			if (!n_pages)
+				goto out;
 		}
-		if (!n_pages)
-			goto out;
 	}
 
 	/* compute number of contiguous chunks */
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH V1] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-11 10:10 [PATCH V1] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
@ 2023-01-11 10:22 ` Chaitanya Kulkarni
  2023-01-16 16:10 ` Jason Gunthorpe
  1 sibling, 0 replies; 3+ messages in thread
From: Chaitanya Kulkarni @ 2023-01-11 10:22 UTC (permalink / raw
  To: Yishai Hadas
  Cc: hch@lst.de, alex.williamson@redhat.com, willy@infradead.org,
	akpm@linux-foundation.org, logang@deltatee.com, axboe@kernel.dk,
	Jason Gunthorpe, linux-mm@kvack.org, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org, Leon Romanovsky, Maor Gottlieb

On 1/11/23 02:10, Yishai Hadas wrote:
> The last_pg is wrong, it is actually the first page of the last
> scatterlist element. To get the last page of the last scatterlist
> element we have to add prv->length. So it is checking mergability
> against the wrong page, Further, a SG element is not guaranteed to end
> on a page boundary, so we have to check the sub page location also for
> merge eligibility.
> 
> Fix the above by checking physical contiguity based on PFNs, compute the
> actual last page and then call pages_are_mergable().
> 
> Fixes: 1567b49d1a40 ("lib/scatterlist: add check when merging zone device pages")
> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck




^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V1] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-11 10:10 [PATCH V1] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
  2023-01-11 10:22 ` Chaitanya Kulkarni
@ 2023-01-16 16:10 ` Jason Gunthorpe
  1 sibling, 0 replies; 3+ messages in thread
From: Jason Gunthorpe @ 2023-01-16 16:10 UTC (permalink / raw
  To: Yishai Hadas
  Cc: linux-kernel, linux-block, linux-mm, axboe, logang, akpm, willy,
	hch, alex.williamson, leonro, maorg

On Wed, Jan 11, 2023 at 12:10:54PM +0200, Yishai Hadas wrote:
> The last_pg is wrong, it is actually the first page of the last
> scatterlist element. To get the last page of the last scatterlist
> element we have to add prv->length. So it is checking mergability
> against the wrong page, Further, a SG element is not guaranteed to end
> on a page boundary, so we have to check the sub page location also for
> merge eligibility.
> 
> Fix the above by checking physical contiguity based on PFNs, compute the
> actual last page and then call pages_are_mergable().
> 
> Fixes: 1567b49d1a40 ("lib/scatterlist: add check when merging zone device pages")
> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>

Applied to rdma for-rc

Thanks,
Jason

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-01-16 16:19 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-11 10:10 [PATCH V1] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
2023-01-11 10:22 ` Chaitanya Kulkarni
2023-01-16 16:10 ` Jason Gunthorpe

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 read-only IMAP folder(s) and NNTP newsgroup(s).