Re: [PATCH v14 105/138] iomap: Convert iomap_add_to_ioend to take a folio

On Thu, Jul 15, 2021 at 03:01:20PM -0700, Darrick J. Wong wrote:

On Thu, Jul 15, 2021 at 04:36:31AM +0100, Matthew Wilcox (Oracle) wrote:

quoted

 
-	merged = __bio_try_merge_page(wpc->ioend->io_bio, page, len, poff,
-			&same_page);
 	if (iop)
 		atomic_add(len, &iop->write_bytes_pending);
-
-	if (!merged) {
-		if (bio_full(wpc->ioend->io_bio, len)) {
-			wpc->ioend->io_bio =
-				iomap_chain_bio(wpc->ioend->io_bio);
-		}
-		bio_add_page(wpc->ioend->io_bio, page, len, poff);
+	if (!bio_add_folio(wpc->ioend->io_bio, folio, len, poff)) {
+		wpc->ioend->io_bio = iomap_chain_bio(wpc->ioend->io_bio);
+		bio_add_folio(wpc->ioend->io_bio, folio, len, poff);

The paranoiac in me wonders if we ought to have some sort of error
checking here just in case we encounter double failures?

Maybe?  We didn't have it before, and it's just been allocated.
I'd defer to Christoph here.

quoted

-	for (i = 0, file_offset = page_offset(page);
-	     i < (PAGE_SIZE >> inode->i_blkbits) && file_offset < end_offset;
-	     i++, file_offset += len) {
+	for (i = 0; i < nblocks; i++, pos += len) {
+		if (pos >= end_offset)
+			break;

Any particular reason this isn't:

	for (i = 0; i < nblocks && pos < end_offset; i++, pos += len) {

?

Just mild personal preference ... I don't even like having the pos +=
len in there.  But you're maintainer, I'll shuffle that in.

Everything from here on out looks decent to me.

Thanks!