On Wed, May 06, 2020 at 07:56:33AM -0700, Christoph Hellwig wrote:
> On Mon, May 04, 2020 at 06:10:22PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Bail out if there's something not right with either file's fork
> > mappings.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_bmap_util.c | 31 +++++++++++++++++++++++--------
> > 1 file changed, 23 insertions(+), 8 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index cc23a3e23e2d..2774939e176d 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1342,8 +1342,16 @@ xfs_swap_extent_rmap(
> > &nimaps, 0);
> > if (error)
> > goto out;
> > - ASSERT(nimaps == 1);
> > - ASSERT(tirec.br_startblock != DELAYSTARTBLOCK);
> > + if (nimaps != 1 || tirec.br_startblock == DELAYSTARTBLOCK) {
> > + /*
> > + * We should never get no mapping or a delalloc extent
> > + * since the donor file should have been flushed by the
> > + * caller.
> > + */
> > + ASSERT(0);
> > + error = -EINVAL;
> > + goto out;
> > + }
>
> I'm not even sure the !nimaps case still exists. Usually this will
> return a hole extent, which we don't seem to handle here?
xfs_bmap_unmap_extent and xfs_bmap_map_extent are NOPs if you pass
them a hole.
But yeah, the !nimaps case doesn't seem to exist anymore.
> In general I think this code would be improved quite a bit by using
> xfs_iext_lookup_extent instead of xfs_bmapi_read.
>
> Same for the second hunk.
I'd rather not make major changes to this code because my long range
plan is to replace all this with a much cleaner implementation in the
atomic file update series[1]. This patchset bandages the wtfs that I
found while writing that series, projecting that review of atomic file
updates is going to take a while...
--D
[1] https://lwn.net/ml/linux-fsdevel/158812825316.168506.932540609191384366.stgit@magnolia/
