On Wed, Jul 19, 2017 at 07:17:41AM -0400, Brian Foster wrote:
> On Wed, Jul 19, 2017 at 09:12:02AM +1000, Dave Chinner wrote:
> > On Tue, Jul 18, 2017 at 10:13:37AM -0400, Brian Foster wrote:
> > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > ---
> > >
> > > Hi all,
> > >
> > > This patch is actually targeted at userspace. The previous change in commit
> > > f3d7ebde ("xfs: fix superblock inprogress check") to use ->b_maps technically
> > > breaks the logic in userspace in a similar way to the original problem because
> > > userspace has no concept of uncached buffers. ->b_maps is NULL in userspace
> > > unless the buffer is truly discontiguous.
> > >
> > > This would normally result in a segfault but this appears to be hidden
> > > by gcc optimization as -O2 is enabled by default and the
> > > check_inprogress param to xfs_mount_validate_sb() is unused in
> > > userspace. Therefore, the segfault is only reproducible when
> > > optimization is disabled (which is a useful configuration for
> > > debugging).
> > >
> > > There are obviously different ways to fix this. I'm floating this (untested)
> > > rfc as a kernel patch (do we ever sync libxfs from xfsprogs -> kernel?) with
> > > the objective of keeping the libxfs code the same between the kernel and
> > > userspace. We could alternatively create a custom helper/macro with the
> > > appropriate check in each place. Thoughts?
> >
> > Wouldn't it be better to simply fix the userspace buffer
> > initialisation to always have a valid bp->b_maps, just like the
> > kernel does? (See xfs_buf_get_maps() in the kernel code). That way
> > we don't have a landmine lurking in all the shared libxfs code we
> > bring from the kernel that may interact with uncached buffers.
> >
>
> We could certainly create a bp->__b_map field in xfsprogs libxfs and
> initialize ->b_maps similar to the kernel for nmap == 1 buffers. Given
> the lack of overlap of uncached buffers between xfsprogs and the kernel
> (I'm not sure there are other cases where such buffers are commonly
> handled), I don't personally think one way is notably better than the
> other.
>
> The tradeoffs seem to be that this patch is fairly localized but leaves
> the potentially different states for uncached buffers in kernel vs.
> xfsprogs context. The above approach addresses that issue at the cost of
> slightly increasing the size of xfs_buf in userspace for something that
> may not ever be necessary outside of an isolated bit of code. It also
> only requires a change to xfsprogs libxfs.
>
> Given the tradeoffs, I have no real preference on which approach we
> take. Do you prefer the latter? If so and there are no other objections,
> I'll send a patch along those lines.
I'd prefer the latter (the bp->__b_map solution) simply so we don't
have to worry about it in future. The closer the kernel and
userspace buffer caches are in terms of behaviour, implementation
and structure members the less likely we are to have problems
related to the kernel using uncached buffers...
FWIW, my wish list contains porting the kernel side buffer cache
implementation to userspace to solve the scalabilty problems in the
current userspace implementation that are exposed when repair
hammers multiple AGs at once. Hence anything that gets kernel +
userspace closer together helps get us (minutely) closer to that
goal....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
