small fixes for the always_cow mode

small fixes for the always_cow mode

[Christoph Hellwig]

Hi all,

two little fixes for the always_cow debug mode, found while doing
early feature development based on it.

[PATCH 1/2] xfs: disable xfs_ioc_space for always COW inodes

[Christoph Hellwig]

If we always have to write out of place preallocating blocks is
pointless.  We already check for this in the normal falloc path, but
the check was missig in the legacy ALLOCSP path.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d58f0d6a699e..abf7a102376f 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -33,6 +33,7 @@
 #include "xfs_sb.h"
 #include "xfs_ag.h"
 #include "xfs_health.h"
+#include "xfs_reflink.h"
 
 #include <linux/mount.h>
 #include <linux/namei.h>
@@ -607,6 +608,9 @@ xfs_ioc_space(
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 
+	if (xfs_is_always_cow_inode(ip))
+		return -EOPNOTSUPP;
+
 	if (filp->f_flags & O_DSYNC)
 		flags |= XFS_PREALLOC_SYNC;
 	if (filp->f_mode & FMODE_NOCMTIME)
-- 
2.20.1

Re: [PATCH 1/2] xfs: disable xfs_ioc_space for always COW inodes

[Darrick J. Wong]

On Fri, Oct 11, 2019 at 03:03:15PM +0200, Christoph Hellwig wrote:
> If we always have to write out of place preallocating blocks is
> pointless.  We already check for this in the normal falloc path, but
> the check was missig in the legacy ALLOCSP path.

This function handles other things than preallocation, such as
XFS_IOC_ZERO_RANGE and XFS_IOC_UNRESVSP, which call xfs_zero_file_space
and xfs_free_file_space, respectively.  We don't prohibit fallocate
from calling those two functions on an always_cow inode, so why do that
here?

--D

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d58f0d6a699e..abf7a102376f 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -33,6 +33,7 @@
>  #include "xfs_sb.h"
>  #include "xfs_ag.h"
>  #include "xfs_health.h"
> +#include "xfs_reflink.h"
>  
>  #include <linux/mount.h>
>  #include <linux/namei.h>
> @@ -607,6 +608,9 @@ xfs_ioc_space(
>  	if (!S_ISREG(inode->i_mode))
>  		return -EINVAL;
>  
> +	if (xfs_is_always_cow_inode(ip))
> +		return -EOPNOTSUPP;
> +
>  	if (filp->f_flags & O_DSYNC)
>  		flags |= XFS_PREALLOC_SYNC;
>  	if (filp->f_mode & FMODE_NOCMTIME)
> -- 
> 2.20.1
> 

Re: [PATCH 1/2] xfs: disable xfs_ioc_space for always COW inodes

[Christoph Hellwig]

On Fri, Oct 11, 2019 at 05:29:54PM -0700, Darrick J. Wong wrote:
> On Fri, Oct 11, 2019 at 03:03:15PM +0200, Christoph Hellwig wrote:
> > If we always have to write out of place preallocating blocks is
> > pointless.  We already check for this in the normal falloc path, but
> > the check was missig in the legacy ALLOCSP path.
> 
> This function handles other things than preallocation, such as
> XFS_IOC_ZERO_RANGE and XFS_IOC_UNRESVSP, which call xfs_zero_file_space
> and xfs_free_file_space, respectively.  We don't prohibit fallocate
> from calling those two functions on an always_cow inode, so why do that
> here?

True.  I actually have a patch in my tree that switches those to
be handled in the core so that they enter XFS through ->fallocate.
It didn't make any sense to send this patch before that other change,
sorry.

[PATCH 2/2] xfs: ignore extent size hints for always COW inodes

[Christoph Hellwig]

There is no point in applying extent size hints for always COW inodes,
as we would just have to COW any extra allocation beyond the data
actually written.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 18f4b262e61c..2e94deb4610a 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -55,6 +55,12 @@ xfs_extlen_t
 xfs_get_extsz_hint(
 	struct xfs_inode	*ip)
 {
+	/*
+	 * No point in aligning allocations if we need to COW to actually
+	 * write to them.
+	 */
+	if (xfs_is_always_cow_inode(ip))
+		return 0;
 	if ((ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE) && ip->i_d.di_extsize)
 		return ip->i_d.di_extsize;
 	if (XFS_IS_REALTIME_INODE(ip))
-- 
2.20.1

Re: [PATCH 2/2] xfs: ignore extent size hints for always COW inodes

[Darrick J. Wong]

On Fri, Oct 11, 2019 at 06:03:16AM -0700, Christoph Hellwig wrote:
> There is no point in applying extent size hints for always COW inodes,
> as we would just have to COW any extra allocation beyond the data
> actually written.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok, I guess?

By the way, what's the plan for always_cow inodes, seeing as it's still
only a debugging feature?

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_inode.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 18f4b262e61c..2e94deb4610a 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -55,6 +55,12 @@ xfs_extlen_t
>  xfs_get_extsz_hint(
>  	struct xfs_inode	*ip)
>  {
> +	/*
> +	 * No point in aligning allocations if we need to COW to actually
> +	 * write to them.
> +	 */
> +	if (xfs_is_always_cow_inode(ip))
> +		return 0;
>  	if ((ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE) && ip->i_d.di_extsize)
>  		return ip->i_d.di_extsize;
>  	if (XFS_IS_REALTIME_INODE(ip))
> -- 
> 2.20.1
> 

Re: [PATCH 2/2] xfs: ignore extent size hints for always COW inodes

[Christoph Hellwig]

On Fri, Oct 11, 2019 at 05:32:26PM -0700, Darrick J. Wong wrote:
> On Fri, Oct 11, 2019 at 06:03:16AM -0700, Christoph Hellwig wrote:
> > There is no point in applying extent size hints for always COW inodes,
> > as we would just have to COW any extra allocation beyond the data
> > actually written.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Looks ok, I guess?
> 
> By the way, what's the plan for always_cow inodes, seeing as it's still
> only a debugging feature?

Support for zoned devices and an O_ATOMIC-like mode that supports
data integrity safe overwrites.  I've found some time to spend on
both lately, but the former might land on the list first.