On Wed, Feb 20, 2019 at 04:08:18PM +0100, Christoph Hellwig wrote:
> On Tue, Feb 19, 2019 at 10:31:14AM -0800, Darrick J. Wong wrote:
> > > - xfs/326 fails to modify the file at all in always_cow mode after
> > > injecting the refcount error, leading to an unexpected md5sum
> > > after the remount, but that again is expected
> >
> > I'm assuming the purpose of the debug knob is so that we can get all the
> > fstests regressions fixed before enabling always cow modes for users...
>
> Theoriginal purpose is to allow exercising the always COW functionality,
> so that it can be tested without SMR support or atomic write which are
> still in flux.
>
> While developing it I also noticed that it also helps to uncover general
> COW functionality bugs, so it also is a pretty good debug aid for that.
>
> > > + } else {
> > > + /*
> > > + * If always_cow mode we can't use preallocations and
> > > + * thus should not create them.
> > > + */
> > > + if (xfs_is_always_cow_inode(ip)) {
> > > + error = -EOPNOTSUPP;
> > > + goto out_unlock;
> > > + }
> > > +
> >
> > Ok, so the basic premise here is that we never call xfs_alloc_file_space
> > for alwayscow inodes. As I pointed out in my reply that focused on
> > fstests errors, xfs_file_fallocate is not the only caller of
> > xfs_alloc_file_space, so I think xfs_ioc_space and friends are going to
> > need a similar treatment.
>
> As mentioned the XFS_IOC_RESVSP / XFS_IOC_RESVSP64 cases in
> xfs_ioc_space are dead code. the allocspc/freespc ones aren't, and
> while those aren't super useful on always_cow inodes they do work
> just fine.
>
> > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > > index 7b71dcc97ef0..72533e9dddc6 100644
> > > --- a/fs/xfs/xfs_iomap.c
> > > +++ b/fs/xfs/xfs_iomap.c
> > > @@ -395,12 +395,13 @@ xfs_quota_calc_throttle(
> > > STATIC xfs_fsblock_t
> > > xfs_iomap_prealloc_size(
> > > struct xfs_inode *ip,
> > > + int whichfork,
> > > loff_t offset,
> > > loff_t count,
> > > struct xfs_iext_cursor *icur)
> > > {
> > > struct xfs_mount *mp = ip->i_mount;
> > > - struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > > + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> >
> > Hmm, so are we able to do preallocations in the cow fork now?
> >
> > I think that's a separate change from adding the alwayscow knob.
>
> I can split a prep patch out, but by always writing through the COW fork
> in always_cow mode we also want the speculative EOF preallocations there.
> Without always_cow we don't COW at EOF per defintion so it is rather
> pointless.
>
> > > +static inline bool xfs_is_always_cow_inode(struct xfs_inode *ip)
> > > +{
> > > + return ip->i_mount->m_always_cow &&
> > > + xfs_sb_version_hasreflink(&ip->i_mount->m_sb);
> >
> > Assuming the next step is to rewrite this function to autodetect a
> > device that cannot easily handle rewrites (aka SMR)...
>
> For SMR we'll have a per-sb flag, for atomic writes a per-inode one.
> But right now I intended these to exist in addition, not instead of the
> debug know.
>
> > > --- a/fs/xfs/xfs_sysctl.h
> > > +++ b/fs/xfs/xfs_sysctl.h
> > > @@ -85,6 +85,7 @@ struct xfs_globals {
> > > int log_recovery_delay; /* log recovery delay (secs) */
> > > int mount_delay; /* mount setup delay (secs) */
> > > bool bug_on_assert; /* BUG() the kernel on assert failure */
> > > + bool always_cow; /* use COW fork for all overwrites */
> >
> > ...I'm also wondering if it makes sense to separate the creation of the
> > sysctl knob into a separate patch so that we can cleanly revert just
> > this part whenever we land a more permanent interface for enabling
> > alwayscow paths?
>
> I could split it - but my intention was to keep the debug know around
> in the long term, as it is really helpful for exercising the COW code.
/me decides he'll put this one in since it's a debug knob...
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
