diff mbox series

[PATCHv9,4/6] iomap: Refactor iomap_write_delalloc_punch() function out

Message ID 62950460a9e78804df28c548327d779a8d53243f.1686395560.git.ritesh.list@gmail.com (mailing list archive)
State Superseded
Headers show
Series iomap: Add support for per-block dirty state to improve write performance | expand

Commit Message

Ritesh Harjani (IBM) June 10, 2023, 11:39 a.m. UTC 
This patch factors iomap_write_delalloc_punch() function out. This function
is resposible for actual punch out operation.
The reason for doing this is, to avoid deep indentation when we bring punch-out
of individual non-dirty blocks within a dirty folio in a later patch
(which adds per-block dirty status handling to iomap) to avoid delalloc block
leak.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/iomap/buffered-io.c | 54 ++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 20 deletions(-)

Comments

Christoph Hellwig June 12, 2023, 6:25 a.m. UTC | #1 
On Sat, Jun 10, 2023 at 05:09:05PM +0530, Ritesh Harjani (IBM) wrote:
> This patch factors iomap_write_delalloc_punch() function out. This function
> is resposible for actual punch out operation.
> The reason for doing this is, to avoid deep indentation when we bring punch-out
> of individual non-dirty blocks within a dirty folio in a later patch
> (which adds per-block dirty status handling to iomap) to avoid delalloc block
> leak.

A bunch of overly long lines in the commit message here,
but otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Ritesh Harjani (IBM) June 12, 2023, 9:01 a.m. UTC | #2 
Christoph Hellwig <hch@infradead.org> writes:

> On Sat, Jun 10, 2023 at 05:09:05PM +0530, Ritesh Harjani (IBM) wrote:
>> This patch factors iomap_write_delalloc_punch() function out. This function
>> is resposible for actual punch out operation.
>> The reason for doing this is, to avoid deep indentation when we bring punch-out
>> of individual non-dirty blocks within a dirty folio in a later patch
>> (which adds per-block dirty status handling to iomap) to avoid delalloc block
>> leak.
>
> A bunch of overly long lines in the commit message here,
> but otherwise this looks good:

Sure. Will shorten those in next revision (considering we might need it for
some minor changes in patch-6)

>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

-ritesh
Matthew Wilcox June 12, 2023, 1:22 p.m. UTC | #3 
On Sat, Jun 10, 2023 at 05:09:05PM +0530, Ritesh Harjani (IBM) wrote:
> +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
> +		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
> +		int (*punch)(struct inode *inode, loff_t offset, loff_t length))

I can't help but feel that a

typedef iomap_punch_t(struct inode *, loff_t offset, loff_t length);

would make all of this easier to read.

> +	/*
> +	 * Make sure the next punch start is correctly bound to
> +	 * the end of this data range, not the end of the folio.
> +	 */
> +	*punch_start_byte = min_t(loff_t, end_byte,
> +				  folio_next_index(folio) << PAGE_SHIFT);

	*punch_start_byte = min(end_byte, folio_pos(folio) + folio_size(folio));
Pankaj Raghav June 12, 2023, 1:56 p.m. UTC | #4 
Minor nit:

> +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
> +		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
> +		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
> +{
> +	int ret = 0;
> +
> +	if (!folio_test_dirty(folio))
> +		return ret;
Either this could be changed to `goto out`

OR

> +
> +	/* if dirty, punch up to offset */
> +	if (start_byte > *punch_start_byte) {
> +		ret = punch(inode, *punch_start_byte,
> +				start_byte - *punch_start_byte);
> +		if (ret)
> +			goto out;

This could be changed to `return ret` and we could get rid of the `out`
label.
> +	}
> +	/*
> +	 * Make sure the next punch start is correctly bound to
> +	 * the end of this data range, not the end of the folio.
> +	 */
> +	*punch_start_byte = min_t(loff_t, end_byte,
> +				  folio_next_index(folio) << PAGE_SHIFT);
> +
> +out:
> +	return ret;
> +}
> +
Ritesh Harjani (IBM) June 12, 2023, 2:03 p.m. UTC | #5 
Matthew Wilcox <willy@infradead.org> writes:

> On Sat, Jun 10, 2023 at 05:09:05PM +0530, Ritesh Harjani (IBM) wrote:
>> +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
>> +		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
>> +		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
>
> I can't help but feel that a
>
> typedef iomap_punch_t(struct inode *, loff_t offset, loff_t length);
>
> would make all of this easier to read.
>

Sure. Make sense.

>> +	/*
>> +	 * Make sure the next punch start is correctly bound to
>> +	 * the end of this data range, not the end of the folio.
>> +	 */
>> +	*punch_start_byte = min_t(loff_t, end_byte,
>> +				  folio_next_index(folio) << PAGE_SHIFT);
>
> 	*punch_start_byte = min(end_byte, folio_pos(folio) + folio_size(folio));

Current code was also correct only. But I guess this just avoids
min_t/loff_t thing. No other reason right?

-ritesh
Matthew Wilcox June 12, 2023, 2:19 p.m. UTC | #6 
On Mon, Jun 12, 2023 at 07:33:29PM +0530, Ritesh Harjani wrote:
> >> +	*punch_start_byte = min_t(loff_t, end_byte,
> >> +				  folio_next_index(folio) << PAGE_SHIFT);
> >
> > 	*punch_start_byte = min(end_byte, folio_pos(folio) + folio_size(folio));
> 
> Current code was also correct only. But I guess this just avoids
> min_t/loff_t thing. No other reason right?

Actually, it's buggy on 32-bit platforms.  folio_next_index returns
an unsigned long (32 bit), which is shifted by PAGE_SHIFT, losing the
top bits, then cast to an loff_t.  folio_pos() does this correctly.
Ritesh Harjani (IBM) June 12, 2023, 2:55 p.m. UTC | #7 
Pankaj Raghav <p.raghav@samsung.com> writes:

> Minor nit:
>
>> +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
>> +		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
>> +		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
>> +{
>> +	int ret = 0;
>> +
>> +	if (!folio_test_dirty(folio))
>> +		return ret;
> Either this could be changed to `goto out`
>
> OR
>
>> +
>> +	/* if dirty, punch up to offset */
>> +	if (start_byte > *punch_start_byte) {
>> +		ret = punch(inode, *punch_start_byte,
>> +				start_byte - *punch_start_byte);
>> +		if (ret)
>> +			goto out;
>
> This could be changed to `return ret` and we could get rid of the `out`
> label.

Sure, thanks Pankaj. I noted that too.
Since there is nothing in the out label. So mostly will simply return
ret. Will fix it in the next rev.

-ritesh
diff mbox series

Patch

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 206808f6e818..1261f26479af 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -888,6 +888,33 @@  iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 }
 EXPORT_SYMBOL_GPL(iomap_file_buffered_write);
 
+static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
+		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
+		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
+{
+	int ret = 0;
+
+	if (!folio_test_dirty(folio))
+		return ret;
+
+	/* if dirty, punch up to offset */
+	if (start_byte > *punch_start_byte) {
+		ret = punch(inode, *punch_start_byte,
+				start_byte - *punch_start_byte);
+		if (ret)
+			goto out;
+	}
+	/*
+	 * Make sure the next punch start is correctly bound to
+	 * the end of this data range, not the end of the folio.
+	 */
+	*punch_start_byte = min_t(loff_t, end_byte,
+				  folio_next_index(folio) << PAGE_SHIFT);
+
+out:
+	return ret;
+}
+
 /*
  * Scan the data range passed to us for dirty page cache folios. If we find a
  * dirty folio, punch out the preceeding range and update the offset from which
@@ -911,6 +938,7 @@  static int iomap_write_delalloc_scan(struct inode *inode,
 {
 	while (start_byte < end_byte) {
 		struct folio	*folio;
+		int ret;
 
 		/* grab locked page */
 		folio = filemap_lock_folio(inode->i_mapping,
@@ -921,26 +949,12 @@  static int iomap_write_delalloc_scan(struct inode *inode,
 			continue;
 		}
 
-		/* if dirty, punch up to offset */
-		if (folio_test_dirty(folio)) {
-			if (start_byte > *punch_start_byte) {
-				int	error;
-
-				error = punch(inode, *punch_start_byte,
-						start_byte - *punch_start_byte);
-				if (error) {
-					folio_unlock(folio);
-					folio_put(folio);
-					return error;
-				}
-			}
-
-			/*
-			 * Make sure the next punch start is correctly bound to
-			 * the end of this data range, not the end of the folio.
-			 */
-			*punch_start_byte = min_t(loff_t, end_byte,
-					folio_next_index(folio) << PAGE_SHIFT);
+		ret = iomap_write_delalloc_punch(inode, folio, punch_start_byte,
+						 start_byte, end_byte, punch);
+		if (ret) {
+			folio_unlock(folio);
+			folio_put(folio);
+			return ret;
 		}
 
 		/* move offset to start of next folio in range */