Re: [PATCH v5 10/11] xfs: prepare xfs_break_layouts() for another layout type
>
> +int
> +xfs_break_layouts(
> + struct inode*inode,
> + uint*iolock,
> + unsigned long flags)
> +{
> + struct xfs_inode*ip = XFS_I(inode);
> + uintiolock_assert = 0;
> + int ret = 0;
> +
> + if (flags & XFS_BREAK_REMOTE)
> + iolock_assert |= XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL;
> + if (flags & XFS_BREAK_MAPS)
> + iolock_assert |= XFS_MMAPLOCK_EXCL;
> +
> + ASSERT(xfs_isilocked(ip, iolock_assert));
> +
> + if (flags & XFS_BREAK_REMOTE)
> + ret = xfs_break_leased_layouts(inode, iolock);
> + return ret;
This just looks weird as hell. We already pass in what to drop/reacquire
in the iolock argument. I don't think we need another argument controlled
by the same callers to assert it.
> @@ -768,7 +790,7 @@ xfs_file_fallocate(
> struct xfs_inode*ip = XFS_I(inode);
> longerror;
> enum xfs_prealloc_flags flags = 0;
> - uintiolock = XFS_IOLOCK_EXCL;
> + uintiolock = XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL;
This is a behavior change that should not be in a patch titled
"prepare xfs_break_layouts() for another layout type" but in one
explicitly changing this and documenting why.
In summary: I think this should be replaced with a patch that
allows xfs_break_layouts to be called with the mmap lock held, and
change the callers that want the mmap lock to pass it with a good
explanation, and we should get rid of the XFS_BREAK_* flags here.
(need to check the next patch if there is any other good reason for
them to be added later, though).
Re: [PATCH v5 10/11] xfs: prepare xfs_break_layouts() for another layout type
>
> +int
> +xfs_break_layouts(
> + struct inode*inode,
> + uint*iolock,
> + unsigned long flags)
> +{
> + struct xfs_inode*ip = XFS_I(inode);
> + uintiolock_assert = 0;
> + int ret = 0;
> +
> + if (flags & XFS_BREAK_REMOTE)
> + iolock_assert |= XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL;
> + if (flags & XFS_BREAK_MAPS)
> + iolock_assert |= XFS_MMAPLOCK_EXCL;
> +
> + ASSERT(xfs_isilocked(ip, iolock_assert));
> +
> + if (flags & XFS_BREAK_REMOTE)
> + ret = xfs_break_leased_layouts(inode, iolock);
> + return ret;
This just looks weird as hell. We already pass in what to drop/reacquire
in the iolock argument. I don't think we need another argument controlled
by the same callers to assert it.
> @@ -768,7 +790,7 @@ xfs_file_fallocate(
> struct xfs_inode*ip = XFS_I(inode);
> longerror;
> enum xfs_prealloc_flags flags = 0;
> - uintiolock = XFS_IOLOCK_EXCL;
> + uintiolock = XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL;
This is a behavior change that should not be in a patch titled
"prepare xfs_break_layouts() for another layout type" but in one
explicitly changing this and documenting why.
In summary: I think this should be replaced with a patch that
allows xfs_break_layouts to be called with the mmap lock held, and
change the callers that want the mmap lock to pass it with a good
explanation, and we should get rid of the XFS_BREAK_* flags here.
(need to check the next patch if there is any other good reason for
them to be added later, though).
