On Wed, Jun 03, 2020 at 07:38:09AM +1000, Dave Chinner wrote:
> 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.
>
Seems reasonable enough to me:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
