On Mon, Oct 19, 2020 at 12:55:19PM -0400, Brian Foster wrote:

On Thu, Oct 15, 2020 at 10:49:01AM +0100, Christoph Hellwig wrote:

quoted

quoted

+iomap_zero_range_skip_uncached(struct inode *inode, loff_t *pos,
+		loff_t *count, loff_t *written)
+{
+	unsigned dirty_offset, bytes = 0;
+
+	dirty_offset = page_cache_seek_hole_data(inode, *pos, *count,
+				SEEK_DATA);
+	if (dirty_offset == -ENOENT)
+		bytes = *count;
+	else if (dirty_offset > *pos)
+		bytes = dirty_offset - *pos;
+
+	if (bytes) {
+		*pos += bytes;
+		*count -= bytes;
+		*written += bytes;
+	}

I find the calling conventions weird.  why not return bytes and
keep the increments/decrements of the three variables in the caller?

No particular reason. IIRC I had it both ways and just landed on this.
I'd change it, but as mentioned in the patch 1 thread I don't think this
patch is sufficient (with or without patch 1) anyways because the page
can also have been reclaimed before we get here.

Christoph,

What do you think about introducing behavior specific to
iomap_truncate_page() to unconditionally write zeroes over unwritten
extents? AFAICT that addresses the race and was historical XFS behavior
(via block_truncate_page()) before iomap, so is not without precedent.
What I'd probably do is bury the caller's did_zero parameter into a new
internal struct iomap_zero_data to pass down into
iomap_zero_range_actor(), then extend that structure with a
'zero_unwritten' field such that iomap_zero_range_actor() can do this:

        if (srcmap->type == IOMAP_HOLE ||
            (srcmap->type == IOMAP_UNWRITTEN && !zdata->zero_unwritten))
                return count;

iomap_truncate_page() would set that flag either via open coding
iomap_zero_range() or creating a new internal wrapper. Hm?

Brian

Brian