Re: [PATCH v2 00/16] block atomic writes

John Garry
2023-12-19T12:44:14Z

On 19/12/2023 05:21, Christoph Hellwig wrote:
> On Mon, Dec 18, 2023 at 09:14:56PM -0800, Darrick J. Wong wrote:
>> /me stumbles back in with plenty of covidbrain to go around.
>>
>> So ... Christoph, you're asking for a common API for
>> sysadmins/applications to signal to the filesystem that they want all
>> data allocations aligned to a given value, right?
>>
>> e.g. if a nvme device advertises a capability for untorn writes between
>> $lbasize and 64k, then we need a way to set up each untorn-file with
>> some alignment between $lbasize and 64k?
>>
>> or if cxl storage becomes not ung-dly expensive, then we'd want a way to
>> set up files with an alignment of 2M (x86) or 512M (arm64lol) to take
>> advantage of PMD mmap mappings?
>
> The most important point is to not mix these up.
>
> If we want to use a file for atomic writes I should tell the fs about
> it, and preferably in a way that does not require me to know about weird
> internal implementation details of every file system. I really just
> want to use atomic writes. Preferably I'd just start using them after
> asking for the limits. But that's obviously not going to work for
> file systems that use the hardware offload and don't otherwise align
> to the limit (which would suck for very small files anyway :))
>
> So as a compromise I tell the file system before writing or otherwise
> adding any data [1] to file that I want to use atomic writes so that
> the fs can take the right decisions.
>
> [1] reflinking data into a such marked file will be ... interesting.
>

How about something based on fcntl, like below? We will prob also
require some per-FS flag for enabling atomic writes without HW support.
That flag might be also useful for XFS for differentiating forcealign
for atomic writes with just forcealign.

---8<----

diff --git a/fs/fcntl.c b/fs/fcntl.c
index c80a6acad742..d4a50655565a 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -313,6 +313,15 @@ static long fcntl_rw_hint(struct file *file,
unsigned int cmd,
}
}

+static long fcntl_atomic_write(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ if (file->f_op->enable_atomic_writes)
+ return file->f_op->enable_atomic_writes(file, arg);
+
+ return -EOPNOTSUPP;
+}
+
static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
struct file *filp)
{
@@ -419,6 +428,9 @@ static long do_fcntl(int fd, unsigned int cmd,
unsigned long arg,
case F_SET_RW_HINT:
err = fcntl_rw_hint(filp, cmd, arg);
break;
+ case F_SET_ATOMIC_WRITE_SIZE:
+ err = fcntl_atomic_write(filp, cmd, arg);
+ break;
default:
break;
}
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index e33e5e13b95f..dba206cbe1ab 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1478,6 +1478,46 @@ xfs_file_mmap(
return 0;
}

+STATIC int
+xfs_enable_atomic_writes(
+ struct file *file,
+ unsigned int unit_max)
+{
+ struct xfs_inode *ip = XFS_I(file_inode(file));
+ struct mnt_idmap *idmap = file_mnt_idmap(file);
+ struct dentry *dentry = file->f_path.dentry;
+ struct xfs_buftarg *target = xfs_inode_buftarg(ip);
+ struct block_device *bdev = target->bt_bdev;
+ struct request_queue *q = bdev->bd_queue;
+ struct inode *inode = &ip->i_vnode;
+
+ if (queue_atomic_write_unit_max_bytes(q) == 0) {
+ if (unit_max != 0) {
+ /* We expect unbounded CoW size if no HW support */
+ return -ENOTBLK;
+ }
+ /* Do something for CoW support ... */
+
+ return 0;
+ }
+
+ if (!unit_max)
+ return -EINVAL;
+
+ /* bodge alert */
+ if (inode->i_op->fileattr_set) {
+ struct fileattr fa = {
+ .fsx_extsize = unit_max,
+ .fsx_xflags = FS_XFLAG_EXTSIZE | FS_XFLAG_FORCEALIGN,
+ .fsx_valid = 1,
+ };
+
+ return inode->i_op->fileattr_set(idmap, dentry, &fa);
+ }
+
+ return -EOPNOTSUPP;
+}
+
const struct file_operations xfs_file_operations = {
.llseek = xfs_file_llseek,
.read_iter = xfs_file_read_iter,
@@ -1498,6 +1538,7 @@ const struct file_operations xfs_file_operations = {
.fallocate = xfs_file_fallocate,
.fadvise = xfs_file_fadvise,
.remap_file_range = xfs_file_remap_range,
+ .enable_atomic_writes = xfs_enable_atomic_writes,
};

const struct file_operations xfs_dir_file_operations = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..a715f98057b8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1956,6 +1956,7 @@ struct file_operations {
int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
unsigned int poll_flags);
+ int (*enable_atomic_writes)(struct file *, unsigned int unit_max);
} __randomize_layout;

/* Wrap a directory iterator that needs exclusive inode access */
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 6c80f96049bd..69780c49622e 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -56,6 +56,8 @@
#define F_GET_FILE_RW_HINT (F_LINUX_SPECIFIC_BASE + 13)
#define F_SET_FILE_RW_HINT (F_LINUX_SPECIFIC_BASE + 14)

+#define F_SET_ATOMIC_WRITE_SIZE (F_LINUX_SPECIFIC_BASE + 15)
+
/*
* Valid hint values for F_{GET,SET}_RW_HINT. 0 is "not set", or can be
* used to clear any hints previously set.
--

--->8----