On Tue, Jun 02, 2020 at 12:47:42PM -0400, Brian Foster wrote:
> On Tue, Jun 02, 2020 at 07:42:28AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > All unmarked dirty buffers should be in the AIL and have log items
> > attached to them. Hence when they are written, we will run a
> > callback to remove the item from the AIL if appropriate. Now that
> > we've handled inode and dquot buffers, all remaining calls are to
> > xfs_buf_iodone() and so we can hard code this rather than use an
> > indirect call.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/xfs/xfs_buf.c | 24 ++++++++----------------
> > fs/xfs/xfs_buf.h | 6 +-----
> > fs/xfs/xfs_buf_item.c | 40 ++++++++++------------------------------
> > fs/xfs/xfs_buf_item.h | 4 ++--
> > fs/xfs/xfs_trans_buf.c | 13 +++----------
> > 5 files changed, 24 insertions(+), 63 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 0a69de674af9d..d7695b638e994 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> ...
> > @@ -1226,14 +1225,7 @@ xfs_buf_ioend(
> > xfs_buf_dquot_iodone(bp);
> > return;
> > }
> > -
> > - if (bp->b_iodone) {
> > - (*(bp->b_iodone))(bp);
> > - return;
> > - }
> > -
> > -out_finish:
> > - xfs_buf_ioend_finish(bp);
> > + xfs_buf_iodone(bp);
>
> The way this function ends up would probably look nicer as an if/else
> chain rather than a sequence of internal return statements.
I've kinda avoided refactoring these early patches because they
cascade into non-trivial conflicts with later patches in the series.
I've spent too much time chasing bugs introduced in the later
patches because of conflict resolution not being quite right. Hence
I want to leave cleanup and refactoring to a series after this whole
line of development is complete and the problems are solved.
> BTW, is there a longer term need to have three separate iodone functions
> here that do the same thing?
The inode iodone function changes almost immediately. I did it this
way so that the process of changing the inode buffer completion
functionality did not, in any way, impact on other types of buffers.
We need to go through the same process with dquot buffers, and then
once that is done, we can look to refactor all this into a more
integrated solution that largely sits in xfs_buf.c.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
