Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1703603 > unrolled thread

[PATCH v2 0/5] fs, xfs: block map immutable files for dax, dma-to-storage, and swap

Started byDan Williams <dan.j.williams@intel.com>
First post2017-08-04 04:40 +0200
Last post2017-08-05 02:00 +0200
Articles 20 on this page of 27 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/5] fs, xfs: block map immutable files for dax,  dma-to-storage, and swap Dan Williams <dan.j.williams@intel.com> - 2017-08-04 04:40 +0200
    [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP Dan Williams <dan.j.williams@intel.com> - 2017-08-04 04:40 +0200
      Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-04 21:50 +0200
        Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP Dan Williams <dan.j.williams@intel.com> - 2017-08-04 22:00 +0200
      Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP Dave Chinner <david@fromorbit.com> - 2017-08-05 01:40 +0200
        Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP Dan Williams <dan.j.williams@intel.com> - 2017-08-05 01:50 +0200
          Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP Dave Chinner <david@fromorbit.com> - 2017-08-05 02:10 +0200
    [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE Dan Williams <dan.j.williams@intel.com> - 2017-08-04 04:40 +0200
      Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-04 22:10 +0200
        Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE Dan Williams <dan.j.williams@intel.com> - 2017-08-04 22:40 +0200
      Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE Christoph Hellwig <hch@lst.de> - 2017-08-05 11:50 +0200
        Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE Dave Chinner <david@fromorbit.com> - 2017-08-07 02:30 +0200
          Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE Christoph Hellwig <hch@lst.de> - 2017-08-11 12:40 +0200
    Re: [PATCH v2 0/5] fs, xfs: block map immutable files for dax,  dma-to-storage, and swap Dan Williams <dan.j.williams@intel.com> - 2017-08-04 04:40 +0200
      Re: [PATCH v2 0/5] fs, xfs: block map immutable files for dax,         dma-to-storage, and swap Christoph Hellwig <hch@lst.de> - 2017-08-05 12:00 +0200
        Re: [PATCH v2 0/5] fs, xfs: block map immutable files for dax,  dma-to-storage, and swap Dan Williams <dan.j.williams@intel.com> - 2017-08-06 21:00 +0200
          Re: [PATCH v2 0/5] fs, xfs: block map immutable files for dax,         dma-to-storage, and swap Christoph Hellwig <hch@lst.de> - 2017-08-11 12:50 +0200
    [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response  to fallocate Dan Williams <dan.j.williams@intel.com> - 2017-08-04 04:40 +0200
      Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in  response to fallocate "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-04 22:20 +0200
        Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in  response to fallocate Dan Williams <dan.j.williams@intel.com> - 2017-08-04 22:50 +0200
          Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in  response to fallocate Dan Williams <dan.j.williams@intel.com> - 2017-08-04 23:00 +0200
          Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in  response to fallocate "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-04 23:00 +0200
    [PATCH v2 4/5] xfs: introduce XFS_DIFLAG2_IOMAP_IMMUTABLE Dan Williams <dan.j.williams@intel.com> - 2017-08-04 04:40 +0200
      Re: [PATCH v2 4/5] xfs: introduce XFS_DIFLAG2_IOMAP_IMMUTABLE "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-04 22:40 +0200
        Re: [PATCH v2 4/5] xfs: introduce XFS_DIFLAG2_IOMAP_IMMUTABLE Dan Williams <dan.j.williams@intel.com> - 2017-08-04 22:50 +0200
        Re: [PATCH v2 4/5] xfs: introduce XFS_DIFLAG2_IOMAP_IMMUTABLE Dave Chinner <david@fromorbit.com> - 2017-08-05 01:50 +0200
          Re: [PATCH v2 4/5] xfs: introduce XFS_DIFLAG2_IOMAP_IMMUTABLE "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-05 02:00 +0200

Page 1 of 2  [1] 2  Next page →

#1703603 — [PATCH v2 0/5] fs, xfs: block map immutable files for dax, dma-to-storage, and swap

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-04 04:40 +0200
Subject[PATCH v2 0/5] fs, xfs: block map immutable files for dax, dma-to-storage, and swap
Message-ID<uaAGR-Pk-3@gated-at.bofh.it>
Changes since v1 [1]:
* Add IS_IOMAP_IMMUTABLE() checks to xfs ioctl paths that perform block
  map changes (xfs_alloc_file_space and xfs_free_file_space) (Darrick)

* Rather than complete a partial write, fail all writes that would
  attempt to extend the file size (Darrick)

* Introduce FALLOC_FL_UNSEAL_BLOCK_MAP as an explicit operation type for
  clearing S_IOMAP_IMMUTABLE (Dave)

* Rework xfs_seal_file_space() to first complete hole-fill and unshare
  operations and then check the file for suitability under
  XFS_ILOCK_EXCL. (Darrick)

* Add an FS_XFLAG_IOMAP_IMMUTABLE flag so the immutable state can be
  seen by xfs_io. (Dave)

* Move the setting of S_IOMAP_IMMUTABLE to be atomic with respect to the
  successful transaction that records XFS_DIFLAG2_IOMAP_IMMUTABLE.
  (Darrick, Dave)

* Switch to a 'goto out_unlock' style in xfs_seal_file_space() to
  cleanup 'if / else' tree, and use the mapping_mapped() helper. (Dave)

* Rely on XFS_MMAPLOCK_EXCL for reading a stable state of
  mapping->i_mmap. (Dave)

[1]: http://marc.info/?l=linux-fsdevel&m=150135785712967&w=2

---

The daxfile proposal a few weeks back [2] sought to piggy back on the
swapfile implementation to approximate a block map immutable file. This
is an idea Dave originated last year to solve the dax "flush from
userspace" problem [3].

The discussion yielded several results. First, Christoph pointed out
that swapfiles are subtly broken [4].  Second, Darrick [5] and Dave [6]
proposed how to properly implement a block map immutable file.  Finally,
Dave identified some improvements to swapfiles that can be built on the
block-map-immutable mechanism. These patches seek to implement the first
part of the proposal and save the swapfile work to build on top once the
base mechanism is complete.

While the initial motivation for this feature is support for
byte-addressable updates of persistent memory and managing cache
maintenance from userspace, the applications of the feature are broader.
In addition to being the start of a better swapfile mechanism it can
also support a DMA-to-storage use case.  This use case enables
data-acquisition hardware to DMA directly to a storage device address
while being safe in the knowledge that storage mappings will not change.

[2]: https://lkml.org/lkml/2017/6/16/790
[3]: https://lkml.org/lkml/2016/9/11/159
[4]: https://lkml.org/lkml/2017/6/18/31
[5]: https://lkml.org/lkml/2017/6/20/49
[6]: https://www.spinics.net/lists/linux-xfs/msg07871.html

---

Dan Williams (5):
      fs, xfs: introduce S_IOMAP_IMMUTABLE
      fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP
      fs, xfs: introduce FALLOC_FL_UNSEAL_BLOCK_MAP
      xfs: introduce XFS_DIFLAG2_IOMAP_IMMUTABLE
      xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate


 fs/attr.c                   |   10 ++
 fs/open.c                   |   22 +++++
 fs/read_write.c             |    3 +
 fs/xfs/libxfs/xfs_format.h  |    5 +
 fs/xfs/xfs_bmap_util.c      |  181 +++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_bmap_util.h      |    5 +
 fs/xfs/xfs_file.c           |   16 +++-
 fs/xfs/xfs_inode.c          |    2 
 fs/xfs/xfs_ioctl.c          |    7 ++
 fs/xfs/xfs_iops.c           |    8 +-
 include/linux/falloc.h      |    4 +
 include/linux/fs.h          |    2 
 include/uapi/linux/falloc.h |   20 +++++
 include/uapi/linux/fs.h     |    1 
 mm/filemap.c                |    5 +
 15 files changed, 282 insertions(+), 9 deletions(-)

[toc] | [next] | [standalone]

#1703604 — [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-04 04:40 +0200
Subject[PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP
Message-ID<uaAGR-Pk-9@gated-at.bofh.it>
In reply to#1703603
>From falloc.h:

    FALLOC_FL_SEAL_BLOCK_MAP is used to seal (make immutable) all of the
    file logical-to-physical extent offset mappings in the file. The
    purpose is to allow an application to assume that there are no holes
    or shared extents in the file and that the metadata needed to find
    all the physical extents of the file is stable and can never be
    dirtied.

For now this patch only permits setting the in-memory state of
S_IOMAP_IMMMUTABLE. Support for clearing and persisting the state is
saved for later patches.

The implementation is careful to not allow the immutable state to change
while any process might have any established mappings. It reuses the
existing xfs_reflink_unshare() and xfs_alloc_file_space() to unshare
extents and fill all holes in the file. It then holds XFS_ILOCK_EXCL
while it validates the file is in the proper state and sets
S_IOMAP_IMMUTABLE.

Cc: Jan Kara <jack@suse.cz>
Cc: Jeff Moyer <jmoyer@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Suggested-by: Dave Chinner <david@fromorbit.com>
Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/open.c                   |   11 +++++
 fs/xfs/xfs_bmap_util.c      |  101 +++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_bmap_util.h      |    2 +
 fs/xfs/xfs_file.c           |   14 ++++--
 include/linux/falloc.h      |    3 +
 include/uapi/linux/falloc.h |   19 ++++++++
 6 files changed, 145 insertions(+), 5 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 7395860d7164..e3aae59785ae 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -273,6 +273,17 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	    (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE)))
 		return -EINVAL;
 
+	/*
+	 * Seal block map operation should only be used exclusively, and
+	 * with the IMMUTABLE capability.
+	 */
+	if (mode & FALLOC_FL_SEAL_BLOCK_MAP) {
+		if (!capable(CAP_LINUX_IMMUTABLE))
+			return -EPERM;
+		if (mode & ~FALLOC_FL_SEAL_BLOCK_MAP)
+			return -EINVAL;
+	}
+
 	if (!(file->f_mode & FMODE_WRITE))
 		return -EBADF;
 
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index fe0f8f7f4bb7..46d8eb9e19fc 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1393,6 +1393,107 @@ xfs_zero_file_space(
 
 }
 
+/* Return 1 if hole detected, 0 if not, and < 0 if fail to determine */
+STATIC int
+xfs_file_has_holes(
+	struct xfs_inode	*ip)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_bmbt_irec	*map;
+	const int		map_size = 10;	/* constrain memory overhead */
+	int			i, nmaps;
+	int			error = 0;
+	xfs_fileoff_t		lblkno = 0;
+	xfs_filblks_t		maxlblkcnt;
+
+	map = kmem_alloc(map_size * sizeof(*map), KM_SLEEP);
+
+	maxlblkcnt = XFS_B_TO_FSB(mp, i_size_read(VFS_I(ip)));
+	do {
+		nmaps = map_size;
+		error = xfs_bmapi_read(ip, lblkno, maxlblkcnt - lblkno,
+				       map, &nmaps, 0);
+		if (error)
+			break;
+
+		ASSERT(nmaps <= map_size);
+		for (i = 0; i < nmaps; i++) {
+			lblkno += map[i].br_blockcount;
+			if (map[i].br_startblock == HOLESTARTBLOCK) {
+				error = 1;
+				break;
+			}
+		}
+	} while (nmaps > 0 && error == 0);
+
+	kmem_free(map);
+	return error;
+}
+
+int
+xfs_seal_file_space(
+	struct xfs_inode	*ip,
+	xfs_off_t		offset,
+	xfs_off_t		len)
+{
+	struct inode		*inode = VFS_I(ip);
+	struct address_space	*mapping = inode->i_mapping;
+	int			error;
+
+	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+
+	if (offset)
+		return -EINVAL;
+
+	error = xfs_reflink_unshare(ip, offset, len);
+	if (error)
+		return error;
+
+	error = xfs_alloc_file_space(ip, offset, len,
+			XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO);
+	if (error)
+		return error;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	/*
+	 * Either the size changed after we performed allocation /
+	 * unsharing, or the request was too small to begin with.
+	 */
+	error = -EINVAL;
+	if (len < i_size_read(inode))
+		goto out_unlock;
+
+	/*
+	 * Allow DAX path to assume that the state of S_IOMAP_IMMUTABLE
+	 * will never change while any mapping is established.
+	 */
+	error = -EBUSY;
+	if (mapping_mapped(mapping))
+		goto out_unlock;
+
+	/* Did we race someone attempting to share extents? */
+	if (xfs_is_reflink_inode(ip))
+		goto out_unlock;
+
+	/* Did we race a hole punch? */
+	error = xfs_file_has_holes(ip);
+	if (error == 1) {
+		error = -EBUSY;
+		goto out_unlock;
+	}
+
+	/* Abort on an error reading the block map */
+	if (error < 0)
+		goto out_unlock;
+
+	inode->i_flags |= S_IOMAP_IMMUTABLE;
+
+out_unlock:
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+
+	return error;
+}
+
 /*
  * @next_fsb will keep track of the extent currently undergoing shift.
  * @stop_fsb will keep track of the extent at which we have to stop.
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index 0cede1043571..5115a32a2483 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -60,6 +60,8 @@ int	xfs_collapse_file_space(struct xfs_inode *, xfs_off_t offset,
 				xfs_off_t len);
 int	xfs_insert_file_space(struct xfs_inode *, xfs_off_t offset,
 				xfs_off_t len);
+int	xfs_seal_file_space(struct xfs_inode *, xfs_off_t offset,
+				xfs_off_t len);
 
 /* EOF block manipulation functions */
 bool	xfs_can_free_eofblocks(struct xfs_inode *ip, bool force);
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c4893e226fd8..e21121530a90 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -739,7 +739,8 @@ xfs_file_write_iter(
 #define	XFS_FALLOC_FL_SUPPORTED						\
 		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
 		 FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |	\
-		 FALLOC_FL_INSERT_RANGE | FALLOC_FL_UNSHARE_RANGE)
+		 FALLOC_FL_INSERT_RANGE | FALLOC_FL_UNSHARE_RANGE |	\
+		 FALLOC_FL_SEAL_BLOCK_MAP)
 
 STATIC long
 xfs_file_fallocate(
@@ -834,9 +835,14 @@ xfs_file_fallocate(
 				error = xfs_reflink_unshare(ip, offset, len);
 				if (error)
 					goto out_unlock;
-			}
-			error = xfs_alloc_file_space(ip, offset, len,
-						     XFS_BMAPI_PREALLOC);
+
+				error = xfs_alloc_file_space(ip, offset, len,
+						XFS_BMAPI_PREALLOC);
+			} else if (mode & FALLOC_FL_SEAL_BLOCK_MAP) {
+				error = xfs_seal_file_space(ip, offset, len);
+			} else
+				error = xfs_alloc_file_space(ip, offset, len,
+						XFS_BMAPI_PREALLOC);
 		}
 		if (error)
 			goto out_unlock;
diff --git a/include/linux/falloc.h b/include/linux/falloc.h
index 7494dc67c66f..48546c6fbec7 100644
--- a/include/linux/falloc.h
+++ b/include/linux/falloc.h
@@ -26,6 +26,7 @@ struct space_resv {
 					 FALLOC_FL_COLLAPSE_RANGE |	\
 					 FALLOC_FL_ZERO_RANGE |		\
 					 FALLOC_FL_INSERT_RANGE |	\
-					 FALLOC_FL_UNSHARE_RANGE)
+					 FALLOC_FL_UNSHARE_RANGE |	\
+					 FALLOC_FL_SEAL_BLOCK_MAP)
 
 #endif /* _FALLOC_H_ */
diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h
index b075f601919b..39076975bf6f 100644
--- a/include/uapi/linux/falloc.h
+++ b/include/uapi/linux/falloc.h
@@ -76,4 +76,23 @@
  */
 #define FALLOC_FL_UNSHARE_RANGE		0x40
 
+/*
+ * FALLOC_FL_SEAL_BLOCK_MAP is used to seal (make immutable) all of the
+ * file logical-to-physical extent offset mappings in the file. The
+ * purpose is to allow an application to assume that there are no holes
+ * or shared extents in the file and that the metadata needed to find
+ * all the physical extents of the file is stable and can never be
+ * dirtied.
+ *
+ * The immutable property is in effect for the entire inode, so the
+ * range for this operation must start at offset 0 and len must be
+ * greater than or equal to the current size of the file. If greater,
+ * this operation allocates, unshares, hole fills, and seals in one
+ * atomic step. If len is zero then the immutable state is cleared for
+ * the inode.
+ *
+ * This flag implies FALLOC_FL_UNSHARE_RANGE and as such cannot be used
+ * with the punch, zero, collapse, or insert range modes.
+ */
+#define FALLOC_FL_SEAL_BLOCK_MAP	0x080
 #endif /* _UAPI_FALLOC_H_ */

[toc] | [prev] | [next] | [standalone]

#1704182 — Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP

From"Darrick J. Wong" <darrick.wong@oracle.com>
Date2017-08-04 21:50 +0200
SubjectRe: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP
Message-ID<uaQLE-2Qu-7@gated-at.bofh.it>
In reply to#1703604
On Thu, Aug 03, 2017 at 07:28:17PM -0700, Dan Williams wrote:
> >From falloc.h:
> 
>     FALLOC_FL_SEAL_BLOCK_MAP is used to seal (make immutable) all of the
>     file logical-to-physical extent offset mappings in the file. The
>     purpose is to allow an application to assume that there are no holes
>     or shared extents in the file and that the metadata needed to find
>     all the physical extents of the file is stable and can never be
>     dirtied.
> 
> For now this patch only permits setting the in-memory state of
> S_IOMAP_IMMMUTABLE. Support for clearing and persisting the state is
> saved for later patches.
> 
> The implementation is careful to not allow the immutable state to change
> while any process might have any established mappings. It reuses the
> existing xfs_reflink_unshare() and xfs_alloc_file_space() to unshare
> extents and fill all holes in the file. It then holds XFS_ILOCK_EXCL
> while it validates the file is in the proper state and sets
> S_IOMAP_IMMUTABLE.
> 
> Cc: Jan Kara <jack@suse.cz>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  fs/open.c                   |   11 +++++
>  fs/xfs/xfs_bmap_util.c      |  101 +++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_bmap_util.h      |    2 +
>  fs/xfs/xfs_file.c           |   14 ++++--
>  include/linux/falloc.h      |    3 +
>  include/uapi/linux/falloc.h |   19 ++++++++
>  6 files changed, 145 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 7395860d7164..e3aae59785ae 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -273,6 +273,17 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>  	    (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE)))
>  		return -EINVAL;
>  
> +	/*
> +	 * Seal block map operation should only be used exclusively, and
> +	 * with the IMMUTABLE capability.
> +	 */
> +	if (mode & FALLOC_FL_SEAL_BLOCK_MAP) {
> +		if (!capable(CAP_LINUX_IMMUTABLE))
> +			return -EPERM;
> +		if (mode & ~FALLOC_FL_SEAL_BLOCK_MAP)
> +			return -EINVAL;
> +	}
> +
>  	if (!(file->f_mode & FMODE_WRITE))
>  		return -EBADF;
>  
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index fe0f8f7f4bb7..46d8eb9e19fc 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1393,6 +1393,107 @@ xfs_zero_file_space(
>  
>  }
>  
> +/* Return 1 if hole detected, 0 if not, and < 0 if fail to determine */
> +STATIC int
> +xfs_file_has_holes(
> +	struct xfs_inode	*ip)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_bmbt_irec	*map;
> +	const int		map_size = 10;	/* constrain memory overhead */
> +	int			i, nmaps;
> +	int			error = 0;
> +	xfs_fileoff_t		lblkno = 0;
> +	xfs_filblks_t		maxlblkcnt;
> +
> +	map = kmem_alloc(map_size * sizeof(*map), KM_SLEEP);

Sleeping with an inode fully locked and (eventually) a running
transaction?  Yikes.

Just allocate one xfs_bmbt_irec on the stack and pass in nmaps=1.

This method might fit better in libxfs/xfs_bmap.c where it'll be
able to scan the extent list more quickly with the iext helpers.

> +
> +	maxlblkcnt = XFS_B_TO_FSB(mp, i_size_read(VFS_I(ip)));
> +	do {
> +		nmaps = map_size;
> +		error = xfs_bmapi_read(ip, lblkno, maxlblkcnt - lblkno,
> +				       map, &nmaps, 0);
> +		if (error)
> +			break;
> +
> +		ASSERT(nmaps <= map_size);
> +		for (i = 0; i < nmaps; i++) {
> +			lblkno += map[i].br_blockcount;
> +			if (map[i].br_startblock == HOLESTARTBLOCK) {

I think we also need to check for unwritten extents here, because a
write to an unwritten block requires some zeroing and a mapping metdata
update.

> +				error = 1;
> +				break;
> +			}
> +		}
> +	} while (nmaps > 0 && error == 0);
> +
> +	kmem_free(map);
> +	return error;
> +}
> +
> +int
> +xfs_seal_file_space(
> +	struct xfs_inode	*ip,
> +	xfs_off_t		offset,
> +	xfs_off_t		len)
> +{
> +	struct inode		*inode = VFS_I(ip);
> +	struct address_space	*mapping = inode->i_mapping;
> +	int			error;
> +
> +	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));

The IOLOCK must be held here too.

> +
> +	if (offset)
> +		return -EINVAL;
> +
> +	error = xfs_reflink_unshare(ip, offset, len);
> +	if (error)
> +		return error;
> +
> +	error = xfs_alloc_file_space(ip, offset, len,
> +			XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO);
> +	if (error)
> +		return error;
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	/*
> +	 * Either the size changed after we performed allocation /
> +	 * unsharing, or the request was too small to begin with.
> +	 */
> +	error = -EINVAL;
> +	if (len < i_size_read(inode))
> +		goto out_unlock;
> +
> +	/*
> +	 * Allow DAX path to assume that the state of S_IOMAP_IMMUTABLE
> +	 * will never change while any mapping is established.
> +	 */
> +	error = -EBUSY;
> +	if (mapping_mapped(mapping))
> +		goto out_unlock;
> +
> +	/* Did we race someone attempting to share extents? */
> +	if (xfs_is_reflink_inode(ip))
> +		goto out_unlock;
> +
> +	/* Did we race a hole punch? */
> +	error = xfs_file_has_holes(ip);
> +	if (error == 1) {
> +		error = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +	/* Abort on an error reading the block map */
> +	if (error < 0)
> +		goto out_unlock;
> +
> +	inode->i_flags |= S_IOMAP_IMMUTABLE;
> +
> +out_unlock:
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +
> +	return error;
> +}
> +
>  /*
>   * @next_fsb will keep track of the extent currently undergoing shift.
>   * @stop_fsb will keep track of the extent at which we have to stop.
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 0cede1043571..5115a32a2483 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -60,6 +60,8 @@ int	xfs_collapse_file_space(struct xfs_inode *, xfs_off_t offset,
>  				xfs_off_t len);
>  int	xfs_insert_file_space(struct xfs_inode *, xfs_off_t offset,
>  				xfs_off_t len);
> +int	xfs_seal_file_space(struct xfs_inode *, xfs_off_t offset,
> +				xfs_off_t len);
>  
>  /* EOF block manipulation functions */
>  bool	xfs_can_free_eofblocks(struct xfs_inode *ip, bool force);
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c4893e226fd8..e21121530a90 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -739,7 +739,8 @@ xfs_file_write_iter(
>  #define	XFS_FALLOC_FL_SUPPORTED						\
>  		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
>  		 FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |	\
> -		 FALLOC_FL_INSERT_RANGE | FALLOC_FL_UNSHARE_RANGE)
> +		 FALLOC_FL_INSERT_RANGE | FALLOC_FL_UNSHARE_RANGE |	\
> +		 FALLOC_FL_SEAL_BLOCK_MAP)
>  
>  STATIC long
>  xfs_file_fallocate(
> @@ -834,9 +835,14 @@ xfs_file_fallocate(
>  				error = xfs_reflink_unshare(ip, offset, len);
>  				if (error)
>  					goto out_unlock;
> -			}
> -			error = xfs_alloc_file_space(ip, offset, len,
> -						     XFS_BMAPI_PREALLOC);
> +
> +				error = xfs_alloc_file_space(ip, offset, len,
> +						XFS_BMAPI_PREALLOC);
> +			} else if (mode & FALLOC_FL_SEAL_BLOCK_MAP) {
> +				error = xfs_seal_file_space(ip, offset, len);
> +			} else
> +				error = xfs_alloc_file_space(ip, offset, len,
> +						XFS_BMAPI_PREALLOC);
>  		}
>  		if (error)
>  			goto out_unlock;
> diff --git a/include/linux/falloc.h b/include/linux/falloc.h
> index 7494dc67c66f..48546c6fbec7 100644
> --- a/include/linux/falloc.h
> +++ b/include/linux/falloc.h
> @@ -26,6 +26,7 @@ struct space_resv {
>  					 FALLOC_FL_COLLAPSE_RANGE |	\
>  					 FALLOC_FL_ZERO_RANGE |		\
>  					 FALLOC_FL_INSERT_RANGE |	\
> -					 FALLOC_FL_UNSHARE_RANGE)
> +					 FALLOC_FL_UNSHARE_RANGE |	\
> +					 FALLOC_FL_SEAL_BLOCK_MAP)
>  
>  #endif /* _FALLOC_H_ */
> diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h
> index b075f601919b..39076975bf6f 100644
> --- a/include/uapi/linux/falloc.h
> +++ b/include/uapi/linux/falloc.h
> @@ -76,4 +76,23 @@
>   */
>  #define FALLOC_FL_UNSHARE_RANGE		0x40
>  
> +/*
> + * FALLOC_FL_SEAL_BLOCK_MAP is used to seal (make immutable) all of the
> + * file logical-to-physical extent offset mappings in the file. The
> + * purpose is to allow an application to assume that there are no holes
> + * or shared extents in the file and that the metadata needed to find
> + * all the physical extents of the file is stable and can never be
> + * dirtied.
> + *
> + * The immutable property is in effect for the entire inode, so the
> + * range for this operation must start at offset 0 and len must be
> + * greater than or equal to the current size of the file. If greater,
> + * this operation allocates, unshares, hole fills, and seals in one

'allocates' is the same as 'hole fills', I think.

This converts unwritten extents to zeroed written extents too, correct?

> + * atomic step. If len is zero then the immutable state is cleared for
> + * the inode.

It's cleared if len == 0?  I thought that was what FL_UNSEAL is for?

> + * This flag implies FALLOC_FL_UNSHARE_RANGE and as such cannot be used
> + * with the punch, zero, collapse, or insert range modes.
> + */
> +#define FALLOC_FL_SEAL_BLOCK_MAP	0x080
>  #endif /* _UAPI_FALLOC_H_ */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1704187 — Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-04 22:00 +0200
SubjectRe: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP
Message-ID<uaQVj-2UY-5@gated-at.bofh.it>
In reply to#1704182
On Fri, Aug 4, 2017 at 12:46 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Thu, Aug 03, 2017 at 07:28:17PM -0700, Dan Williams wrote:
>> >From falloc.h:
>>
>>     FALLOC_FL_SEAL_BLOCK_MAP is used to seal (make immutable) all of the
>>     file logical-to-physical extent offset mappings in the file. The
>>     purpose is to allow an application to assume that there are no holes
>>     or shared extents in the file and that the metadata needed to find
>>     all the physical extents of the file is stable and can never be
>>     dirtied.
>>
>> For now this patch only permits setting the in-memory state of
>> S_IOMAP_IMMMUTABLE. Support for clearing and persisting the state is
>> saved for later patches.
>>
>> The implementation is careful to not allow the immutable state to change
>> while any process might have any established mappings. It reuses the
>> existing xfs_reflink_unshare() and xfs_alloc_file_space() to unshare
>> extents and fill all holes in the file. It then holds XFS_ILOCK_EXCL
>> while it validates the file is in the proper state and sets
>> S_IOMAP_IMMUTABLE.
>>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: Jeff Moyer <jmoyer@redhat.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Suggested-by: Dave Chinner <david@fromorbit.com>
>> Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  fs/open.c                   |   11 +++++
>>  fs/xfs/xfs_bmap_util.c      |  101 +++++++++++++++++++++++++++++++++++++++++++
>>  fs/xfs/xfs_bmap_util.h      |    2 +
>>  fs/xfs/xfs_file.c           |   14 ++++--
>>  include/linux/falloc.h      |    3 +
>>  include/uapi/linux/falloc.h |   19 ++++++++
>>  6 files changed, 145 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/open.c b/fs/open.c
>> index 7395860d7164..e3aae59785ae 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -273,6 +273,17 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>           (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE)))
>>               return -EINVAL;
>>
>> +     /*
>> +      * Seal block map operation should only be used exclusively, and
>> +      * with the IMMUTABLE capability.
>> +      */
>> +     if (mode & FALLOC_FL_SEAL_BLOCK_MAP) {
>> +             if (!capable(CAP_LINUX_IMMUTABLE))
>> +                     return -EPERM;
>> +             if (mode & ~FALLOC_FL_SEAL_BLOCK_MAP)
>> +                     return -EINVAL;
>> +     }
>> +
>>       if (!(file->f_mode & FMODE_WRITE))
>>               return -EBADF;
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index fe0f8f7f4bb7..46d8eb9e19fc 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -1393,6 +1393,107 @@ xfs_zero_file_space(
>>
>>  }
>>
>> +/* Return 1 if hole detected, 0 if not, and < 0 if fail to determine */
>> +STATIC int
>> +xfs_file_has_holes(
>> +     struct xfs_inode        *ip)
>> +{
>> +     struct xfs_mount        *mp = ip->i_mount;
>> +     struct xfs_bmbt_irec    *map;
>> +     const int               map_size = 10;  /* constrain memory overhead */
>> +     int                     i, nmaps;
>> +     int                     error = 0;
>> +     xfs_fileoff_t           lblkno = 0;
>> +     xfs_filblks_t           maxlblkcnt;
>> +
>> +     map = kmem_alloc(map_size * sizeof(*map), KM_SLEEP);
>
> Sleeping with an inode fully locked and (eventually) a running
> transaction?  Yikes.
>
> Just allocate one xfs_bmbt_irec on the stack and pass in nmaps=1.
>
> This method might fit better in libxfs/xfs_bmap.c where it'll be
> able to scan the extent list more quickly with the iext helpers.

Ok, I'll take a look.

>
>> +
>> +     maxlblkcnt = XFS_B_TO_FSB(mp, i_size_read(VFS_I(ip)));
>> +     do {
>> +             nmaps = map_size;
>> +             error = xfs_bmapi_read(ip, lblkno, maxlblkcnt - lblkno,
>> +                                    map, &nmaps, 0);
>> +             if (error)
>> +                     break;
>> +
>> +             ASSERT(nmaps <= map_size);
>> +             for (i = 0; i < nmaps; i++) {
>> +                     lblkno += map[i].br_blockcount;
>> +                     if (map[i].br_startblock == HOLESTARTBLOCK) {
>
> I think we also need to check for unwritten extents here, because a
> write to an unwritten block requires some zeroing and a mapping metdata
> update.

Will do.

>
>> +                             error = 1;
>> +                             break;
>> +                     }
>> +             }
>> +     } while (nmaps > 0 && error == 0);
>> +
>> +     kmem_free(map);
>> +     return error;
>> +}
>> +
>> +int
>> +xfs_seal_file_space(
>> +     struct xfs_inode        *ip,
>> +     xfs_off_t               offset,
>> +     xfs_off_t               len)
>> +{
>> +     struct inode            *inode = VFS_I(ip);
>> +     struct address_space    *mapping = inode->i_mapping;
>> +     int                     error;
>> +
>> +     ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
>
> The IOLOCK must be held here too.

Ok, I can add that. I had this here for the mapping_mapped() check.

>
>> +
>> +     if (offset)
>> +             return -EINVAL;
>> +
>> +     error = xfs_reflink_unshare(ip, offset, len);
>> +     if (error)
>> +             return error;
>> +
>> +     error = xfs_alloc_file_space(ip, offset, len,
>> +                     XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO);
>> +     if (error)
>> +             return error;
>> +
>> +     xfs_ilock(ip, XFS_ILOCK_EXCL);
>> +     /*
>> +      * Either the size changed after we performed allocation /
>> +      * unsharing, or the request was too small to begin with.
>> +      */
>> +     error = -EINVAL;
>> +     if (len < i_size_read(inode))
>> +             goto out_unlock;
>> +
>> +     /*
>> +      * Allow DAX path to assume that the state of S_IOMAP_IMMUTABLE
>> +      * will never change while any mapping is established.
>> +      */
>> +     error = -EBUSY;
>> +     if (mapping_mapped(mapping))
>> +             goto out_unlock;
>> +
>> +     /* Did we race someone attempting to share extents? */
>> +     if (xfs_is_reflink_inode(ip))
>> +             goto out_unlock;
>> +
>> +     /* Did we race a hole punch? */
>> +     error = xfs_file_has_holes(ip);
>> +     if (error == 1) {
>> +             error = -EBUSY;
>> +             goto out_unlock;
>> +     }
>> +
>> +     /* Abort on an error reading the block map */
>> +     if (error < 0)
>> +             goto out_unlock;
>> +
>> +     inode->i_flags |= S_IOMAP_IMMUTABLE;
>> +
>> +out_unlock:
>> +     xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +
>> +     return error;
>> +}
>> +
>>  /*
>>   * @next_fsb will keep track of the extent currently undergoing shift.
>>   * @stop_fsb will keep track of the extent at which we have to stop.
>> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
>> index 0cede1043571..5115a32a2483 100644
>> --- a/fs/xfs/xfs_bmap_util.h
>> +++ b/fs/xfs/xfs_bmap_util.h
>> @@ -60,6 +60,8 @@ int xfs_collapse_file_space(struct xfs_inode *, xfs_off_t offset,
>>                               xfs_off_t len);
>>  int  xfs_insert_file_space(struct xfs_inode *, xfs_off_t offset,
>>                               xfs_off_t len);
>> +int  xfs_seal_file_space(struct xfs_inode *, xfs_off_t offset,
>> +                             xfs_off_t len);
>>
>>  /* EOF block manipulation functions */
>>  bool xfs_can_free_eofblocks(struct xfs_inode *ip, bool force);
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index c4893e226fd8..e21121530a90 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -739,7 +739,8 @@ xfs_file_write_iter(
>>  #define      XFS_FALLOC_FL_SUPPORTED                                         \
>>               (FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |           \
>>                FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |      \
>> -              FALLOC_FL_INSERT_RANGE | FALLOC_FL_UNSHARE_RANGE)
>> +              FALLOC_FL_INSERT_RANGE | FALLOC_FL_UNSHARE_RANGE |     \
>> +              FALLOC_FL_SEAL_BLOCK_MAP)
>>
>>  STATIC long
>>  xfs_file_fallocate(
>> @@ -834,9 +835,14 @@ xfs_file_fallocate(
>>                               error = xfs_reflink_unshare(ip, offset, len);
>>                               if (error)
>>                                       goto out_unlock;
>> -                     }
>> -                     error = xfs_alloc_file_space(ip, offset, len,
>> -                                                  XFS_BMAPI_PREALLOC);
>> +
>> +                             error = xfs_alloc_file_space(ip, offset, len,
>> +                                             XFS_BMAPI_PREALLOC);
>> +                     } else if (mode & FALLOC_FL_SEAL_BLOCK_MAP) {
>> +                             error = xfs_seal_file_space(ip, offset, len);
>> +                     } else
>> +                             error = xfs_alloc_file_space(ip, offset, len,
>> +                                             XFS_BMAPI_PREALLOC);
>>               }
>>               if (error)
>>                       goto out_unlock;
>> diff --git a/include/linux/falloc.h b/include/linux/falloc.h
>> index 7494dc67c66f..48546c6fbec7 100644
>> --- a/include/linux/falloc.h
>> +++ b/include/linux/falloc.h
>> @@ -26,6 +26,7 @@ struct space_resv {
>>                                        FALLOC_FL_COLLAPSE_RANGE |     \
>>                                        FALLOC_FL_ZERO_RANGE |         \
>>                                        FALLOC_FL_INSERT_RANGE |       \
>> -                                      FALLOC_FL_UNSHARE_RANGE)
>> +                                      FALLOC_FL_UNSHARE_RANGE |      \
>> +                                      FALLOC_FL_SEAL_BLOCK_MAP)
>>
>>  #endif /* _FALLOC_H_ */
>> diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h
>> index b075f601919b..39076975bf6f 100644
>> --- a/include/uapi/linux/falloc.h
>> +++ b/include/uapi/linux/falloc.h
>> @@ -76,4 +76,23 @@
>>   */
>>  #define FALLOC_FL_UNSHARE_RANGE              0x40
>>
>> +/*
>> + * FALLOC_FL_SEAL_BLOCK_MAP is used to seal (make immutable) all of the
>> + * file logical-to-physical extent offset mappings in the file. The
>> + * purpose is to allow an application to assume that there are no holes
>> + * or shared extents in the file and that the metadata needed to find
>> + * all the physical extents of the file is stable and can never be
>> + * dirtied.
>> + *
>> + * The immutable property is in effect for the entire inode, so the
>> + * range for this operation must start at offset 0 and len must be
>> + * greater than or equal to the current size of the file. If greater,
>> + * this operation allocates, unshares, hole fills, and seals in one
>
> 'allocates' is the same as 'hole fills', I think.
>
> This converts unwritten extents to zeroed written extents too, correct?

Yes, I'll add that and also include that in the man page patch that
I'm working on...

>
>> + * atomic step. If len is zero then the immutable state is cleared for
>> + * the inode.
>
> It's cleared if len == 0?  I thought that was what FL_UNSEAL is for?

Whoops, stale holdover from v1.

>> + * This flag implies FALLOC_FL_UNSHARE_RANGE and as such cannot be used
>> + * with the punch, zero, collapse, or insert range modes.
>> + */
>> +#define FALLOC_FL_SEAL_BLOCK_MAP     0x080
>>  #endif /* _UAPI_FALLOC_H_ */

Thanks Darrick!

[toc] | [prev] | [next] | [standalone]

#1704373 — Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP

FromDave Chinner <david@fromorbit.com>
Date2017-08-05 01:40 +0200
SubjectRe: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP
Message-ID<uaUmh-5dC-69@gated-at.bofh.it>
In reply to#1703604
On Thu, Aug 03, 2017 at 07:28:17PM -0700, Dan Williams wrote:
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index fe0f8f7f4bb7..46d8eb9e19fc 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1393,6 +1393,107 @@ xfs_zero_file_space(
>  
>  }
>  
> +/* Return 1 if hole detected, 0 if not, and < 0 if fail to determine */
> +STATIC int
> +xfs_file_has_holes(
> +	struct xfs_inode	*ip)
> +{

Why do we need this function?

We've just run xfs_alloc_file_space() across the entire range we
are sealing, so we've already guaranteed that it won't have holes
in it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1704386 — Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-05 01:50 +0200
SubjectRe: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP
Message-ID<uaUvU-5hz-9@gated-at.bofh.it>
In reply to#1704373
On Fri, Aug 4, 2017 at 4:31 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Aug 03, 2017 at 07:28:17PM -0700, Dan Williams wrote:
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index fe0f8f7f4bb7..46d8eb9e19fc 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -1393,6 +1393,107 @@ xfs_zero_file_space(
>>
>>  }
>>
>> +/* Return 1 if hole detected, 0 if not, and < 0 if fail to determine */
>> +STATIC int
>> +xfs_file_has_holes(
>> +     struct xfs_inode        *ip)
>> +{
>
> Why do we need this function?
>
> We've just run xfs_alloc_file_space() across the entire range we
> are sealing, so we've already guaranteed that it won't have holes
> in it.

I'm sure this is due to my ignorance of the scope of XFS_IOLOCK_EXCL
vs XFS_ILOCK_EXCL. I had assumed that since we drop and retake
XFS_ILOCK_EXCL that we need to re-validate the block map before
setting S_IOMAP_IMMUTABLE.

[toc] | [prev] | [next] | [standalone]

#1704483 — Re: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP

FromDave Chinner <david@fromorbit.com>
Date2017-08-05 02:10 +0200
SubjectRe: [PATCH v2 2/5] fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP
Message-ID<uaUPi-5G0-57@gated-at.bofh.it>
In reply to#1704386
On Fri, Aug 04, 2017 at 04:43:50PM -0700, Dan Williams wrote:
> On Fri, Aug 4, 2017 at 4:31 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Aug 03, 2017 at 07:28:17PM -0700, Dan Williams wrote:
> >> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> >> index fe0f8f7f4bb7..46d8eb9e19fc 100644
> >> --- a/fs/xfs/xfs_bmap_util.c
> >> +++ b/fs/xfs/xfs_bmap_util.c
> >> @@ -1393,6 +1393,107 @@ xfs_zero_file_space(
> >>
> >>  }
> >>
> >> +/* Return 1 if hole detected, 0 if not, and < 0 if fail to determine */
> >> +STATIC int
> >> +xfs_file_has_holes(
> >> +     struct xfs_inode        *ip)
> >> +{
> >
> > Why do we need this function?
> >
> > We've just run xfs_alloc_file_space() across the entire range we
> > are sealing, so we've already guaranteed that it won't have holes
> > in it.
> 
> I'm sure this is due to my ignorance of the scope of XFS_IOLOCK_EXCL
> vs XFS_ILOCK_EXCL. I had assumed that since we drop and retake
> XFS_ILOCK_EXCL that we need to re-validate the block map before
> setting S_IOMAP_IMMUTABLE.

THe ILOCK is there to protect the inode metadata when there is
concurrent access through the IO/MMAP lock paths.  However, if we
hold the IOLOCK_EXCL and the MMAPLOCK_EXCL, then nothing can get
through the IO interfaces to modify the data in the file.  This is
required because APIs that directly modify the extent map (e.g.
fallocate, truncate, etc) have to lock out the IO path to ensure
there are no IOs in flight across the range we are manipulating.

Holding these locks also locks out other APIs that modify the extent
map and so effectively nothing else can be accessing or modifying
the extent map while a fallocate or truncate operation is in
progress.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1703605 — [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-04 04:40 +0200
Subject[PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE
Message-ID<uaAGS-Pk-11@gated-at.bofh.it>
In reply to#1703603
An inode with this flag set indicates that the file's block map cannot
be changed from the currently allocated set.

The implementation of toggling the flag and sealing the state of the
extent map is saved for a later patch. The functionality provided by
S_IOMAP_IMMUTABLE, once toggle support is added, will be a superset of
that provided by S_SWAPFILE, and it is targeted to replace it.

For now, only xfs and the core vfs are updated to consider the new flag.

The additional checks that are added for this flag, beyond what we are
already doing for swapfiles, are:
* fail writes that try to extend the file size
* fail attempts to directly change the allocation map via fallocate or
  xfs ioctls. This can be done centrally by blocking
  xfs_alloc_file_space and xfs_free_file_space when the flag is set.

Cc: Jan Kara <jack@suse.cz>
Cc: Jeff Moyer <jmoyer@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Suggested-by: Dave Chinner <david@fromorbit.com>
Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/attr.c              |   10 ++++++++++
 fs/open.c              |    6 ++++++
 fs/read_write.c        |    3 +++
 fs/xfs/xfs_bmap_util.c |    6 ++++++
 fs/xfs/xfs_ioctl.c     |    6 ++++++
 include/linux/fs.h     |    2 ++
 mm/filemap.c           |    5 +++++
 7 files changed, 38 insertions(+)

diff --git a/fs/attr.c b/fs/attr.c
index 135304146120..8573e364bd06 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -112,6 +112,16 @@ EXPORT_SYMBOL(setattr_prepare);
  */
 int inode_newsize_ok(const struct inode *inode, loff_t offset)
 {
+	if (IS_IOMAP_IMMUTABLE(inode)) {
+		/*
+		 * Any size change is disallowed. Size increases may
+		 * dirty metadata that an application is not prepared to
+		 * sync, and a size decrease may expose free blocks to
+		 * in-flight DMA.
+		 */
+		return -ETXTBSY;
+	}
+
 	if (inode->i_size < offset) {
 		unsigned long limit;
 
diff --git a/fs/open.c b/fs/open.c
index 35bb784763a4..7395860d7164 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -292,6 +292,12 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		return -ETXTBSY;
 
 	/*
+	 * We cannot allow any allocation changes on an iomap immutable file
+	 */
+	if (IS_IOMAP_IMMUTABLE(inode))
+		return -ETXTBSY;
+
+	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
diff --git a/fs/read_write.c b/fs/read_write.c
index 0cc7033aa413..dc673be7c7cb 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1706,6 +1706,9 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
 	if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
 		return -ETXTBSY;
 
+	if (IS_IOMAP_IMMUTABLE(inode_in) || IS_IOMAP_IMMUTABLE(inode_out))
+		return -ETXTBSY;
+
 	/* Don't reflink dirs, pipes, sockets... */
 	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
 		return -EISDIR;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 93e955262d07..fe0f8f7f4bb7 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1044,6 +1044,9 @@ xfs_alloc_file_space(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
+	if (IS_IOMAP_IMMUTABLE(VFS_I(ip)))
+		return -ETXTBSY;
+
 	error = xfs_qm_dqattach(ip, 0);
 	if (error)
 		return error;
@@ -1294,6 +1297,9 @@ xfs_free_file_space(
 
 	trace_xfs_free_file_space(ip);
 
+	if (IS_IOMAP_IMMUTABLE(VFS_I(ip)))
+		return -ETXTBSY;
+
 	error = xfs_qm_dqattach(ip, 0);
 	if (error)
 		return error;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index e75c40a47b7d..2e64488bc4de 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1755,6 +1755,12 @@ xfs_ioc_swapext(
 		goto out_put_tmp_file;
 	}
 
+	if (IS_IOMAP_IMMUTABLE(file_inode(f.file)) ||
+	    IS_IOMAP_IMMUTABLE(file_inode(tmp.file))) {
+		error = -EINVAL;
+		goto out_put_tmp_file;
+	}
+
 	/*
 	 * We need to ensure that the fds passed in point to XFS inodes
 	 * before we cast and access them as XFS structures as we have no
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6e1fd5d21248..0a254b768855 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1829,6 +1829,7 @@ struct super_operations {
 #else
 #define S_DAX		0	/* Make all the DAX code disappear */
 #endif
+#define S_IOMAP_IMMUTABLE 16384 /* logical-to-physical extent map is fixed */
 
 /*
  * Note that nosuid etc flags are inode-specific: setting some file-system
@@ -1867,6 +1868,7 @@ struct super_operations {
 #define IS_AUTOMOUNT(inode)	((inode)->i_flags & S_AUTOMOUNT)
 #define IS_NOSEC(inode)		((inode)->i_flags & S_NOSEC)
 #define IS_DAX(inode)		((inode)->i_flags & S_DAX)
+#define IS_IOMAP_IMMUTABLE(inode) ((inode)->i_flags & S_IOMAP_IMMUTABLE)
 
 #define IS_WHITEOUT(inode)	(S_ISCHR(inode->i_mode) && \
 				 (inode)->i_rdev == WHITEOUT_DEV)
diff --git a/mm/filemap.c b/mm/filemap.c
index a49702445ce0..a4105a4c1d69 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2806,6 +2806,11 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
 	if (unlikely(pos >= inode->i_sb->s_maxbytes))
 		return -EFBIG;
 
+	/* Are we about to mutate the block map on an immutable file? */
+	if (IS_IOMAP_IMMUTABLE(inode)
+			&& (pos + iov_iter_count(from) > i_size_read(inode)))
+		return -ETXTBSY;
+
 	iov_iter_truncate(from, inode->i_sb->s_maxbytes - pos);
 	return iov_iter_count(from);
 }

[toc] | [prev] | [next] | [standalone]

#1704192 — Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE

From"Darrick J. Wong" <darrick.wong@oracle.com>
Date2017-08-04 22:10 +0200
SubjectRe: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE
Message-ID<uaR50-3eu-13@gated-at.bofh.it>
In reply to#1703605
On Thu, Aug 03, 2017 at 07:28:10PM -0700, Dan Williams wrote:
> An inode with this flag set indicates that the file's block map cannot
> be changed from the currently allocated set.
> 
> The implementation of toggling the flag and sealing the state of the
> extent map is saved for a later patch. The functionality provided by
> S_IOMAP_IMMUTABLE, once toggle support is added, will be a superset of
> that provided by S_SWAPFILE, and it is targeted to replace it.
> 
> For now, only xfs and the core vfs are updated to consider the new flag.
> 
> The additional checks that are added for this flag, beyond what we are
> already doing for swapfiles, are:
> * fail writes that try to extend the file size
> * fail attempts to directly change the allocation map via fallocate or
>   xfs ioctls. This can be done centrally by blocking
>   xfs_alloc_file_space and xfs_free_file_space when the flag is set.
> 
> Cc: Jan Kara <jack@suse.cz>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  fs/attr.c              |   10 ++++++++++
>  fs/open.c              |    6 ++++++
>  fs/read_write.c        |    3 +++
>  fs/xfs/xfs_bmap_util.c |    6 ++++++
>  fs/xfs/xfs_ioctl.c     |    6 ++++++
>  include/linux/fs.h     |    2 ++
>  mm/filemap.c           |    5 +++++
>  7 files changed, 38 insertions(+)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 135304146120..8573e364bd06 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -112,6 +112,16 @@ EXPORT_SYMBOL(setattr_prepare);
>   */
>  int inode_newsize_ok(const struct inode *inode, loff_t offset)
>  {
> +	if (IS_IOMAP_IMMUTABLE(inode)) {
> +		/*
> +		 * Any size change is disallowed. Size increases may
> +		 * dirty metadata that an application is not prepared to
> +		 * sync, and a size decrease may expose free blocks to
> +		 * in-flight DMA.
> +		 */
> +		return -ETXTBSY;
> +	}
> +
>  	if (inode->i_size < offset) {
>  		unsigned long limit;
>  
> diff --git a/fs/open.c b/fs/open.c
> index 35bb784763a4..7395860d7164 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -292,6 +292,12 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>  		return -ETXTBSY;
>  
>  	/*
> +	 * We cannot allow any allocation changes on an iomap immutable file
> +	 */
> +	if (IS_IOMAP_IMMUTABLE(inode))
> +		return -ETXTBSY;
> +
> +	/*
>  	 * Revalidate the write permissions, in case security policy has
>  	 * changed since the files were opened.
>  	 */
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 0cc7033aa413..dc673be7c7cb 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1706,6 +1706,9 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
>  	if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
>  		return -ETXTBSY;
>  
> +	if (IS_IOMAP_IMMUTABLE(inode_in) || IS_IOMAP_IMMUTABLE(inode_out))
> +		return -ETXTBSY;
> +
>  	/* Don't reflink dirs, pipes, sockets... */
>  	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
>  		return -EISDIR;
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 93e955262d07..fe0f8f7f4bb7 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1044,6 +1044,9 @@ xfs_alloc_file_space(
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> +	if (IS_IOMAP_IMMUTABLE(VFS_I(ip)))
> +		return -ETXTBSY;
> +

Hm.  The 'seal this up' caller in the next patch doesn't check for
ETXTBSY (or if it does I missed that), so if you try to seal an already
sealed file you'll get an error code even though you actually got the
state you wanted.

Second question: How might we handle the situation where a filesystem
/has/ to alter a block mapping?  Hypothetically, if the block layer
tells the fs that some range of storage has gone bad and the fs decides
to punch out that part of the file (or mark it unwritten or whatever) to
avoid a machine check, can we lock out file IO, forcibly remove the
mapping from memory, make whatever block map updates we want, and then
unlock?

(Conceptually, the bmbt rebuilder in the online fsck patchset operates
in a similar manner...)

--D

>  	error = xfs_qm_dqattach(ip, 0);
>  	if (error)
>  		return error;
> @@ -1294,6 +1297,9 @@ xfs_free_file_space(
>  
>  	trace_xfs_free_file_space(ip);
>  
> +	if (IS_IOMAP_IMMUTABLE(VFS_I(ip)))
> +		return -ETXTBSY;
> +
>  	error = xfs_qm_dqattach(ip, 0);
>  	if (error)
>  		return error;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index e75c40a47b7d..2e64488bc4de 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1755,6 +1755,12 @@ xfs_ioc_swapext(
>  		goto out_put_tmp_file;
>  	}
>  
> +	if (IS_IOMAP_IMMUTABLE(file_inode(f.file)) ||
> +	    IS_IOMAP_IMMUTABLE(file_inode(tmp.file))) {
> +		error = -EINVAL;
> +		goto out_put_tmp_file;
> +	}
> +
>  	/*
>  	 * We need to ensure that the fds passed in point to XFS inodes
>  	 * before we cast and access them as XFS structures as we have no
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6e1fd5d21248..0a254b768855 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1829,6 +1829,7 @@ struct super_operations {
>  #else
>  #define S_DAX		0	/* Make all the DAX code disappear */
>  #endif
> +#define S_IOMAP_IMMUTABLE 16384 /* logical-to-physical extent map is fixed */
>  
>  /*
>   * Note that nosuid etc flags are inode-specific: setting some file-system
> @@ -1867,6 +1868,7 @@ struct super_operations {
>  #define IS_AUTOMOUNT(inode)	((inode)->i_flags & S_AUTOMOUNT)
>  #define IS_NOSEC(inode)		((inode)->i_flags & S_NOSEC)
>  #define IS_DAX(inode)		((inode)->i_flags & S_DAX)
> +#define IS_IOMAP_IMMUTABLE(inode) ((inode)->i_flags & S_IOMAP_IMMUTABLE)
>  
>  #define IS_WHITEOUT(inode)	(S_ISCHR(inode->i_mode) && \
>  				 (inode)->i_rdev == WHITEOUT_DEV)
> diff --git a/mm/filemap.c b/mm/filemap.c
> index a49702445ce0..a4105a4c1d69 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2806,6 +2806,11 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
>  	if (unlikely(pos >= inode->i_sb->s_maxbytes))
>  		return -EFBIG;
>  
> +	/* Are we about to mutate the block map on an immutable file? */
> +	if (IS_IOMAP_IMMUTABLE(inode)
> +			&& (pos + iov_iter_count(from) > i_size_read(inode)))
> +		return -ETXTBSY;
> +
>  	iov_iter_truncate(from, inode->i_sb->s_maxbytes - pos);
>  	return iov_iter_count(from);
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1704206 — Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-04 22:40 +0200
SubjectRe: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE
Message-ID<uaRy2-3oq-15@gated-at.bofh.it>
In reply to#1704192
On Fri, Aug 4, 2017 at 1:00 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> On Thu, Aug 03, 2017 at 07:28:10PM -0700, Dan Williams wrote:
>> An inode with this flag set indicates that the file's block map cannot
>> be changed from the currently allocated set.
>>
>> The implementation of toggling the flag and sealing the state of the
>> extent map is saved for a later patch. The functionality provided by
>> S_IOMAP_IMMUTABLE, once toggle support is added, will be a superset of
>> that provided by S_SWAPFILE, and it is targeted to replace it.
>>
>> For now, only xfs and the core vfs are updated to consider the new flag.
>>
>> The additional checks that are added for this flag, beyond what we are
>> already doing for swapfiles, are:
>> * fail writes that try to extend the file size
>> * fail attempts to directly change the allocation map via fallocate or
>>   xfs ioctls. This can be done centrally by blocking
>>   xfs_alloc_file_space and xfs_free_file_space when the flag is set.
>>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: Jeff Moyer <jmoyer@redhat.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Suggested-by: Dave Chinner <david@fromorbit.com>
>> Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  fs/attr.c              |   10 ++++++++++
>>  fs/open.c              |    6 ++++++
>>  fs/read_write.c        |    3 +++
>>  fs/xfs/xfs_bmap_util.c |    6 ++++++
>>  fs/xfs/xfs_ioctl.c     |    6 ++++++
>>  include/linux/fs.h     |    2 ++
>>  mm/filemap.c           |    5 +++++
>>  7 files changed, 38 insertions(+)
>>
>> diff --git a/fs/attr.c b/fs/attr.c
>> index 135304146120..8573e364bd06 100644
>> --- a/fs/attr.c
>> +++ b/fs/attr.c
>> @@ -112,6 +112,16 @@ EXPORT_SYMBOL(setattr_prepare);
>>   */
>>  int inode_newsize_ok(const struct inode *inode, loff_t offset)
>>  {
>> +     if (IS_IOMAP_IMMUTABLE(inode)) {
>> +             /*
>> +              * Any size change is disallowed. Size increases may
>> +              * dirty metadata that an application is not prepared to
>> +              * sync, and a size decrease may expose free blocks to
>> +              * in-flight DMA.
>> +              */
>> +             return -ETXTBSY;
>> +     }
>> +
>>       if (inode->i_size < offset) {
>>               unsigned long limit;
>>
>> diff --git a/fs/open.c b/fs/open.c
>> index 35bb784763a4..7395860d7164 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -292,6 +292,12 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>               return -ETXTBSY;
>>
>>       /*
>> +      * We cannot allow any allocation changes on an iomap immutable file
>> +      */
>> +     if (IS_IOMAP_IMMUTABLE(inode))
>> +             return -ETXTBSY;
>> +
>> +     /*
>>        * Revalidate the write permissions, in case security policy has
>>        * changed since the files were opened.
>>        */
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 0cc7033aa413..dc673be7c7cb 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -1706,6 +1706,9 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
>>       if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
>>               return -ETXTBSY;
>>
>> +     if (IS_IOMAP_IMMUTABLE(inode_in) || IS_IOMAP_IMMUTABLE(inode_out))
>> +             return -ETXTBSY;
>> +
>>       /* Don't reflink dirs, pipes, sockets... */
>>       if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
>>               return -EISDIR;
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 93e955262d07..fe0f8f7f4bb7 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -1044,6 +1044,9 @@ xfs_alloc_file_space(
>>       if (XFS_FORCED_SHUTDOWN(mp))
>>               return -EIO;
>>
>> +     if (IS_IOMAP_IMMUTABLE(VFS_I(ip)))
>> +             return -ETXTBSY;
>> +
>
> Hm.  The 'seal this up' caller in the next patch doesn't check for
> ETXTBSY (or if it does I missed that), so if you try to seal an already
> sealed file you'll get an error code even though you actually got the
> state you wanted.

That's a good point, I'll fix that up.

>
> Second question: How might we handle the situation where a filesystem
> /has/ to alter a block mapping?  Hypothetically, if the block layer
> tells the fs that some range of storage has gone bad and the fs decides
> to punch out that part of the file (or mark it unwritten or whatever) to
> avoid a machine check, can we lock out file IO, forcibly remove the
> mapping from memory, make whatever block map updates we want, and then
> unlock?

It's not clear that the filesystem /has/ to change the block mappings
when the backing media supports error clearing. Unlike bad DRAM ranges
where the address is permanently mapped out, we can clear pmem and
disk errors by writing new data. The bad block can be repaired or
remapped internal to the hardware device.

As far as I can see no amount of fs locking will keep in-flight DMA
from assuming it can continue to write to the storage address it
thought was immutable. So, I think that means that the only error
management that can be expected while the file is immutable is
blkdev_issue_zeroout() to clear the error, or otherwise hope that the
DMA operation can generate the properly sized/aligned write request
that can clear the error.

[toc] | [prev] | [next] | [standalone]

#1704612 — Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE

FromChristoph Hellwig <hch@lst.de>
Date2017-08-05 11:50 +0200
SubjectRe: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE
Message-ID<ub3Sy-3eV-9@gated-at.bofh.it>
In reply to#1703605
NAK^4.

We should not allow users to create immutable files.  We have
proper ways to synchronize I/O, and this is just an invitation
for horrible abuses that should not be allowed, and which we've
always people told not to do.

[toc] | [prev] | [next] | [standalone]

#1705031 — Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE

FromDave Chinner <david@fromorbit.com>
Date2017-08-07 02:30 +0200
SubjectRe: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE
Message-ID<ubE5H-Y2-1@gated-at.bofh.it>
In reply to#1704612
On Sat, Aug 05, 2017 at 11:47:08AM +0200, Christoph Hellwig wrote:
> NAK^4.
> 
> We should not allow users to create immutable files.  We have
> proper ways to synchronize I/O, and this is just an invitation
> for horrible abuses that should not be allowed, and which we've
> always people told not to do.

We've always told people not to do those "horrible abuses" because
of the TOCTOU race conditions inherent in getting accurate
BMAP/FIEMAP information to userspace. However, immutable extent maps
solve the TOCTOU problem and so removes the only *technical* barrier
in the way of using extent maps to implement functionality such as
userspace pNFS servers.

The core requirement for a userspace pNFS block server to be able to
safely export the block map of a file to remote clients is that the
extent map is allocated and will not change while the client has
been granted access to it. Immutable extent maps provide that
functionality to userspace.  However, for this to work, us
filesystem developers have to give up the idea that only the
filesystem can access the storage underlying the filesystem.

I'm not writing this for your benefit, Christoph, but for everyone
else who doesn't know about existing direct remote storage access
protocols and implementations. That is, I'm letting everyone know
we've already had to give up the exclusive storage device access
model...

.... when you implemented the kernel pNFS server code that provides
unknown third parties with the *remote direct access* to the storage
underlying the XFS filesystem.

Yup, we already allow third parties to arbitrate and directly access
to the XFS block device map. That "horrible abuse" was allowed
because it could be done safely via NFSv4 delegations and a new API
that provided a "blocks will always be allocated before a write and
won't change while the remote client has access" guarantee from XFS
to the kernel pNFS server (i.e. ->map_blocks()/->commit_blocks()
export ops and the break_layouts() API).

Immutable extent maps provide userspace with this same guarantee, so
what used to be considered a "horrible abuse" can now be done safely
and without risking data and/or filesystem corruption.  So, really,
calling this an "invitation to horrible abuses that should not be
allowed" ignores the reality that you were the architect that
introduced this "safe remote direct access" model to convert a
"horrible abuse" into a set of safe, supportable operations.

In the end, all I care about is that everyone understands the
technical merits of the proposals being considered rather than
discussion and review being shut down because "Christoph shouted
nasty words at me but I still don't understand why?".....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1709447 — Re: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE

FromChristoph Hellwig <hch@lst.de>
Date2017-08-11 12:40 +0200
SubjectRe: [PATCH v2 1/5] fs, xfs: introduce S_IOMAP_IMMUTABLE
Message-ID<udfwe-2mN-3@gated-at.bofh.it>
In reply to#1705031
On Mon, Aug 07, 2017 at 10:25:02AM +1000, Dave Chinner wrote:
> We've always told people not to do those "horrible abuses" because
> of the TOCTOU race conditions inherent in getting accurate
> BMAP/FIEMAP information to userspace. However, immutable extent maps
> solve the TOCTOU problem and so removes the only *technical* barrier
> in the way of using extent maps to implement functionality such as
> userspace pNFS servers.

For pNFS block/scsi and my upcoming RDMA persistent memory layout?
Hell no - we'll need concepts we can't expose to userspace for them,
and to expose the advanced functionality people are asking for
(reflinks, atomic updates, no stale data exposure) immutable extents
maps won't work at all.

> The core requirement for a userspace pNFS block server to be able to
> safely export the block map of a file to remote clients is that the
> extent map is allocated and will not change while the client has
> been granted access to it.

No.  The core feature for the block layout is to create an unwrittent
extent that we can expose to the client for writing to it and only
marking it as written after commit by converting the extent list.

Now I know you're going to argue that this could work with pre-zeroing
the extents, but for and actual SCSI or NVMe device that will suck
badly.  And for RDMA-like layouts we don't even need the zeroing as
we can control client behavior a lot better because memory registrations
allow much more fine grained control.

Either way we a good notification from the file system to the server
when the extent map changes.

But for either blocks or rdma layout and implementation with the filesystem
in kernel space and the server in user is stupid as they need to interact
closely.  There is a good reason why all successful NFS products have
the server very tightly coupled to the file system, and a userspace <->
kernel barrier does not help with that.

[toc] | [prev] | [next] | [standalone]

#1703606

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-04 04:40 +0200
Message-ID<uaAGS-Pk-17@gated-at.bofh.it>
In reply to#1703603
[ adding linux-api to the cover letter for notification, will send the
full set to linux-api for v3 ]

On Thu, Aug 3, 2017 at 7:28 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> Changes since v1 [1]:
> * Add IS_IOMAP_IMMUTABLE() checks to xfs ioctl paths that perform block
>   map changes (xfs_alloc_file_space and xfs_free_file_space) (Darrick)
>
> * Rather than complete a partial write, fail all writes that would
>   attempt to extend the file size (Darrick)
>
> * Introduce FALLOC_FL_UNSEAL_BLOCK_MAP as an explicit operation type for
>   clearing S_IOMAP_IMMUTABLE (Dave)
>
> * Rework xfs_seal_file_space() to first complete hole-fill and unshare
>   operations and then check the file for suitability under
>   XFS_ILOCK_EXCL. (Darrick)
>
> * Add an FS_XFLAG_IOMAP_IMMUTABLE flag so the immutable state can be
>   seen by xfs_io. (Dave)
>
> * Move the setting of S_IOMAP_IMMUTABLE to be atomic with respect to the
>   successful transaction that records XFS_DIFLAG2_IOMAP_IMMUTABLE.
>   (Darrick, Dave)
>
> * Switch to a 'goto out_unlock' style in xfs_seal_file_space() to
>   cleanup 'if / else' tree, and use the mapping_mapped() helper. (Dave)
>
> * Rely on XFS_MMAPLOCK_EXCL for reading a stable state of
>   mapping->i_mmap. (Dave)
>
> [1]: http://marc.info/?l=linux-fsdevel&m=150135785712967&w=2
>
> ---
>
> The daxfile proposal a few weeks back [2] sought to piggy back on the
> swapfile implementation to approximate a block map immutable file. This
> is an idea Dave originated last year to solve the dax "flush from
> userspace" problem [3].
>
> The discussion yielded several results. First, Christoph pointed out
> that swapfiles are subtly broken [4].  Second, Darrick [5] and Dave [6]
> proposed how to properly implement a block map immutable file.  Finally,
> Dave identified some improvements to swapfiles that can be built on the
> block-map-immutable mechanism. These patches seek to implement the first
> part of the proposal and save the swapfile work to build on top once the
> base mechanism is complete.
>
> While the initial motivation for this feature is support for
> byte-addressable updates of persistent memory and managing cache
> maintenance from userspace, the applications of the feature are broader.
> In addition to being the start of a better swapfile mechanism it can
> also support a DMA-to-storage use case.  This use case enables
> data-acquisition hardware to DMA directly to a storage device address
> while being safe in the knowledge that storage mappings will not change.
>
> [2]: https://lkml.org/lkml/2017/6/16/790
> [3]: https://lkml.org/lkml/2016/9/11/159
> [4]: https://lkml.org/lkml/2017/6/18/31
> [5]: https://lkml.org/lkml/2017/6/20/49
> [6]: https://www.spinics.net/lists/linux-xfs/msg07871.html
>
> ---
>
> Dan Williams (5):
>       fs, xfs: introduce S_IOMAP_IMMUTABLE
>       fs, xfs: introduce FALLOC_FL_SEAL_BLOCK_MAP
>       fs, xfs: introduce FALLOC_FL_UNSEAL_BLOCK_MAP
>       xfs: introduce XFS_DIFLAG2_IOMAP_IMMUTABLE
>       xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate
>
>
>  fs/attr.c                   |   10 ++
>  fs/open.c                   |   22 +++++
>  fs/read_write.c             |    3 +
>  fs/xfs/libxfs/xfs_format.h  |    5 +
>  fs/xfs/xfs_bmap_util.c      |  181 +++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_bmap_util.h      |    5 +
>  fs/xfs/xfs_file.c           |   16 +++-
>  fs/xfs/xfs_inode.c          |    2
>  fs/xfs/xfs_ioctl.c          |    7 ++
>  fs/xfs/xfs_iops.c           |    8 +-
>  include/linux/falloc.h      |    4 +
>  include/linux/fs.h          |    2
>  include/uapi/linux/falloc.h |   20 +++++
>  include/uapi/linux/fs.h     |    1
>  mm/filemap.c                |    5 +
>  15 files changed, 282 insertions(+), 9 deletions(-)

[toc] | [prev] | [next] | [standalone]

#1704615 — Re: [PATCH v2 0/5] fs, xfs: block map immutable files for dax, dma-to-storage, and swap

FromChristoph Hellwig <hch@lst.de>
Date2017-08-05 12:00 +0200
SubjectRe: [PATCH v2 0/5] fs, xfs: block map immutable files for dax, dma-to-storage, and swap
Message-ID<ub42e-3iq-9@gated-at.bofh.it>
In reply to#1703606
On Thu, Aug 03, 2017 at 07:38:11PM -0700, Dan Williams wrote:
> [ adding linux-api to the cover letter for notification, will send the
> full set to linux-api for v3 ]

Just don't send this crap ever again.  All the so called use cases in the
earlier thread were incorrect and highly dangerous.

Promising that the block map is stable is not a useful userspace API,
as it the block map is a complete internal implementation detail.

We've been through this a few times but let me repeat it:  The only
sensible API gurantee is one that is observable and usable.

so Jan's synchronous page fault flag in one form or another makes
perfect sense as it is a clear receipe for the user:  you don't
have to call msync to persist your mmap writes.  This API is not,
it guarantees that the block map does not change, but the application
has absolutely no point of even knowing about the block map.

[toc] | [prev] | [next] | [standalone]

#1704943

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-06 21:00 +0200
Message-ID<ubyWl-5Tz-3@gated-at.bofh.it>
In reply to#1704615
On Sat, Aug 5, 2017 at 2:50 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Aug 03, 2017 at 07:38:11PM -0700, Dan Williams wrote:
>> [ adding linux-api to the cover letter for notification, will send the
>> full set to linux-api for v3 ]
>
> Just don't send this crap ever again.  All the so called use cases in the
> earlier thread were incorrect and highly dangerous.

I usually end up coming around to your position on these types of
debates because you almost always put forward unassailable technical
arguments. So far, you have not in this case.

> Promising that the block map is stable is not a useful userspace API,
> as it the block map is a complete internal implementation detail.

Of course it's a useful API. An application already needs to worry
about the block map, that's why we have fallocate, msync, fiemap
and...

> We've been through this a few times but let me repeat it:  The only
> sensible API gurantee is one that is observable and usable.

I'm missing how block-map immutable files violate this observable and
usable constraint?

> so Jan's synchronous page fault flag in one form or another makes
> perfect sense as it is a clear receipe for the user:  you don't
> have to call msync to persist your mmap writes.  This API is not,
> it guarantees that the block map does not change, but the application
> has absolutely no point of even knowing about the block map.

Jan's approach is great, it should go in, it solves a long standing
problem with dax with the only drawback being potentially
unpredictable latency spikes.

This immutable approach should also go in, it solves the same problem
without the the latency drawback, but yes, with the administrative
overhead of CAP_LINUX_IMMUTABLE. Beyond flush from userspace it also
can be used to solve the swapfile problems you highlighted and it
allows safe ongoing dma to a filesystem-dax mapping beyond what we can
already do with direct-I/O. There is demand for these capabilities
that cannot be satisfied by just hand waving them away as invalid.

The magnitude of opposition to this approach is out of step with the
actual risk.

[toc] | [prev] | [next] | [standalone]

#1709452 — Re: [PATCH v2 0/5] fs, xfs: block map immutable files for dax, dma-to-storage, and swap

FromChristoph Hellwig <hch@lst.de>
Date2017-08-11 12:50 +0200
SubjectRe: [PATCH v2 0/5] fs, xfs: block map immutable files for dax, dma-to-storage, and swap
Message-ID<udfFT-2pX-9@gated-at.bofh.it>
In reply to#1704943
On Sun, Aug 06, 2017 at 11:51:50AM -0700, Dan Williams wrote:
> Of course it's a useful API. An application already needs to worry
> about the block map, that's why we have fallocate, msync, fiemap
> and...

Fallocate and msync do not expose the block map in any way.  Proof:
they work just fine over say nfs.

fiemap does indeed expose the block map, which is the whole point.
But it's a debug tool that we don't event have a man page for.  And
it's not usable for anything else, if only for the fact that it doesn't
tell you what device your returned extents are relative to.

> > We've been through this a few times but let me repeat it:  The only
> > sensible API gurantee is one that is observable and usable.
> 
> I'm missing how block-map immutable files violate this observable and
> usable constraint?

What is the observable behavior of an extent map change?  How can you
describe your immutable extent map behavior so that when I violate
them by e.g. moving one extent to a different place on disk you can
observe that in userspace?

> This immutable approach should also go in, it solves the same problem
> without the the latency drawback,

How is your latency going to be any different from MAP_SYNC on
a fully allocated and pre-zeroed file?

> Beyond flush from userspace it also
> can be used to solve the swapfile problems you highlighted

Which swapfile problem?

> and it
> allows safe ongoing dma to a filesystem-dax mapping beyond what we can
> already do with direct-I/O.

Please explain how this interface allows for any sort of safe userspace
DMA.

[toc] | [prev] | [next] | [standalone]

#1703607 — [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-04 04:40 +0200
Subject[PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate
Message-ID<uaAGS-Pk-19@gated-at.bofh.it>
In reply to#1703603
After validating the state of the file as not having holes, shared
extents, or active mappings try to commit the
XFS_DIFLAG2_IOMAP_IMMUTABLE flag to the on-disk inode metadata. If that
succeeds then allow the S_IOMAP_IMMUTABLE to be set on the vfs inode.

Cc: Jan Kara <jack@suse.cz>
Cc: Jeff Moyer <jmoyer@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Suggested-by: Dave Chinner <david@fromorbit.com>
Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/xfs/xfs_bmap_util.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 70ac2d33ab27..8464c25a2403 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1436,9 +1436,11 @@ xfs_seal_file_space(
 	xfs_off_t		offset,
 	xfs_off_t		len)
 {
+	struct xfs_mount	*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
 	struct address_space	*mapping = inode->i_mapping;
 	int			error;
+	struct xfs_trans	*tp;
 
 	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
 
@@ -1454,6 +1456,10 @@ xfs_seal_file_space(
 	if (error)
 		return error;
 
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
+	if (error)
+		return error;
+
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	/*
 	 * Either the size changed after we performed allocation /
@@ -1486,10 +1492,20 @@ xfs_seal_file_space(
 	if (error < 0)
 		goto out_unlock;
 
+	xfs_trans_ijoin(tp, ip, 0);
+	ip->i_d.di_flags2 |= XFS_DIFLAG2_IOMAP_IMMUTABLE;
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+	error = xfs_trans_commit(tp);
+	tp = NULL; /* nothing to cancel */
+	if (error)
+		goto out_unlock;
+
 	inode->i_flags |= S_IOMAP_IMMUTABLE;
 
 out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	if (tp)
+		xfs_trans_cancel(tp);
 
 	return error;
 }
@@ -1500,15 +1516,21 @@ xfs_unseal_file_space(
 	xfs_off_t		offset,
 	xfs_off_t		len)
 {
+	struct xfs_mount	*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
 	struct address_space	*mapping = inode->i_mapping;
 	int			error;
+	struct xfs_trans	*tp;
 
 	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
 
 	if (offset)
 		return -EINVAL;
 
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
+	if (error)
+		return error;
+
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	/*
 	 * It does not make sense to unseal less than the full range of
@@ -1527,11 +1549,21 @@ xfs_unseal_file_space(
 	if (mapping_mapped(mapping))
 		goto out_unlock;
 
+	xfs_trans_ijoin(tp, ip, 0);
+	ip->i_d.di_flags2 &= ~XFS_DIFLAG2_IOMAP_IMMUTABLE;
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+	error = xfs_trans_commit(tp);
+	tp = NULL; /* nothing to cancel */
+	if (error)
+		goto out_unlock;
+
 	inode->i_flags &= ~S_IOMAP_IMMUTABLE;
 	error = 0;
 
 out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	if (tp)
+		xfs_trans_cancel(tp);
 
 	return error;
 }

[toc] | [prev] | [next] | [standalone]

#1704194 — Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate

From"Darrick J. Wong" <darrick.wong@oracle.com>
Date2017-08-04 22:20 +0200
SubjectRe: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate
Message-ID<uaReF-3hK-7@gated-at.bofh.it>
In reply to#1703607
On Thu, Aug 03, 2017 at 07:28:35PM -0700, Dan Williams wrote:
> After validating the state of the file as not having holes, shared
> extents, or active mappings try to commit the
> XFS_DIFLAG2_IOMAP_IMMUTABLE flag to the on-disk inode metadata. If that
> succeeds then allow the S_IOMAP_IMMUTABLE to be set on the vfs inode.
> 
> Cc: Jan Kara <jack@suse.cz>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  fs/xfs/xfs_bmap_util.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 70ac2d33ab27..8464c25a2403 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1436,9 +1436,11 @@ xfs_seal_file_space(
>  	xfs_off_t		offset,
>  	xfs_off_t		len)
>  {
> +	struct xfs_mount	*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
>  	struct address_space	*mapping = inode->i_mapping;
>  	int			error;
> +	struct xfs_trans	*tp;
>
>  	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
>  
> @@ -1454,6 +1456,10 @@ xfs_seal_file_space(
>  	if (error)
>  		return error;
>  
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	if (error)
> +		return error;
> +
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	/*
>  	 * Either the size changed after we performed allocation /
> @@ -1486,10 +1492,20 @@ xfs_seal_file_space(
>  	if (error < 0)
>  		goto out_unlock;
>  
> +	xfs_trans_ijoin(tp, ip, 0);

FWIW if you change that third parameter to XFS_ILOCK_EXCL then
xfs_trans_commit will do the xfs_iunlock(ip, XFS_ILOCK_EXCL) for you if
the commit succeeds...

> +	ip->i_d.di_flags2 |= XFS_DIFLAG2_IOMAP_IMMUTABLE;
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	error = xfs_trans_commit(tp);
> +	tp = NULL; /* nothing to cancel */
> +	if (error)
> +		goto out_unlock;
> +
>  	inode->i_flags |= S_IOMAP_IMMUTABLE;

...and then you can just return out here.

--D

>  out_unlock:
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	if (tp)
> +		xfs_trans_cancel(tp);
>  
>  	return error;
>  }
> @@ -1500,15 +1516,21 @@ xfs_unseal_file_space(
>  	xfs_off_t		offset,
>  	xfs_off_t		len)
>  {
> +	struct xfs_mount	*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
>  	struct address_space	*mapping = inode->i_mapping;
>  	int			error;
> +	struct xfs_trans	*tp;
>  
>  	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
>  
>  	if (offset)
>  		return -EINVAL;
>  
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	if (error)
> +		return error;
> +
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	/*
>  	 * It does not make sense to unseal less than the full range of
> @@ -1527,11 +1549,21 @@ xfs_unseal_file_space(
>  	if (mapping_mapped(mapping))
>  		goto out_unlock;
>  
> +	xfs_trans_ijoin(tp, ip, 0);
> +	ip->i_d.di_flags2 &= ~XFS_DIFLAG2_IOMAP_IMMUTABLE;
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	error = xfs_trans_commit(tp);
> +	tp = NULL; /* nothing to cancel */
> +	if (error)
> +		goto out_unlock;
> +
>  	inode->i_flags &= ~S_IOMAP_IMMUTABLE;
>  	error = 0;
>  
>  out_unlock:
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	if (tp)
> +		xfs_trans_cancel(tp);
>  
>  	return error;
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1704214 — Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate

FromDan Williams <dan.j.williams@intel.com>
Date2017-08-04 22:50 +0200
SubjectRe: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate
Message-ID<uaRHH-3rS-11@gated-at.bofh.it>
In reply to#1704194
On Fri, Aug 4, 2017 at 1:14 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> On Thu, Aug 03, 2017 at 07:28:35PM -0700, Dan Williams wrote:
>> After validating the state of the file as not having holes, shared
>> extents, or active mappings try to commit the
>> XFS_DIFLAG2_IOMAP_IMMUTABLE flag to the on-disk inode metadata. If that
>> succeeds then allow the S_IOMAP_IMMUTABLE to be set on the vfs inode.
>>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: Jeff Moyer <jmoyer@redhat.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Suggested-by: Dave Chinner <david@fromorbit.com>
>> Suggested-by: "Darrick J. Wong" <darrick.wong@oracle.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  fs/xfs/xfs_bmap_util.c |   32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 70ac2d33ab27..8464c25a2403 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -1436,9 +1436,11 @@ xfs_seal_file_space(
>>       xfs_off_t               offset,
>>       xfs_off_t               len)
>>  {
>> +     struct xfs_mount        *mp = ip->i_mount;
>>       struct inode            *inode = VFS_I(ip);
>>       struct address_space    *mapping = inode->i_mapping;
>>       int                     error;
>> +     struct xfs_trans        *tp;
>>
>>       ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
>>
>> @@ -1454,6 +1456,10 @@ xfs_seal_file_space(
>>       if (error)
>>               return error;
>>
>> +     error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
>> +     if (error)
>> +             return error;
>> +
>>       xfs_ilock(ip, XFS_ILOCK_EXCL);
>>       /*
>>        * Either the size changed after we performed allocation /
>> @@ -1486,10 +1492,20 @@ xfs_seal_file_space(
>>       if (error < 0)
>>               goto out_unlock;
>>
>> +     xfs_trans_ijoin(tp, ip, 0);
>
> FWIW if you change that third parameter to XFS_ILOCK_EXCL then
> xfs_trans_commit will do the xfs_iunlock(ip, XFS_ILOCK_EXCL) for you if
> the commit succeeds...
>
>> +     ip->i_d.di_flags2 |= XFS_DIFLAG2_IOMAP_IMMUTABLE;
>> +     xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>> +     error = xfs_trans_commit(tp);
>> +     tp = NULL; /* nothing to cancel */
>> +     if (error)
>> +             goto out_unlock;
>> +
>>       inode->i_flags |= S_IOMAP_IMMUTABLE;
>
> ...and then you can just return out here.

Do we not need to hold XFS_ILOCK_EXCL over ->i_flags changes, or is
XFS_IOLOCK_EXCL enough?

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web