[Top] [All Lists]

Re: [PATCH 05/25] xfs: introduce xfs_bmapi_read()

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 05/25] xfs: introduce xfs_bmapi_read()
From: Alex Elder <aelder@xxxxxxx>
Date: Fri, 2 Sep 2011 17:23:15 -0500
Cc: <xfs@xxxxxxxxxxx>, Dave Chinner <dchinner@xxxxxxxxxx>
In-reply-to: <20110824060641.292341912@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110824060428.789245205@xxxxxxxxxxxxxxxxxxxxxx> <20110824060641.292341912@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-08-24 at 02:04 -0400, Christoph Hellwig wrote:
> xfs_bmapi() currently handles both extent map reading and
> allocation. As a result, the code is littered with "if (wr)"
> branches to conditionally do allocation operations if required.
> This makes the code much harder to follow and causes significant
> indent issues with the code.
> Given that read mapping is much simpler than allocation, we can
> split out read mapping from xfs_bmapi() and reuse the logic that
> we have already factored out do do all the hard work of handling the
> extent map manipulations. The results in a much simpler function for
> the common extent read operations, and will allow the allocation
> code to be simplified in another commit.
> Once xfs_bmapi_read() is implemented, convert all the callers of
> xfs_bmapi() that are only reading extents to use the new function.
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

Very nice.  About a third of the uses of XFS_BMAPI_METADATA
were unnecessary because that flag only matters on writes.
That's a good demonstration of how refactoring like this
improves clarity (if the simplicity of the xfs_bmapi_read()
function isn't evidence enough).

Reviewed-by: Alex Elder <aelder@xxxxxxx>

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH 05/25] xfs: introduce xfs_bmapi_read(), Alex Elder <=