Re: [PATCH v7 2/9] fs: Initial atomic write support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05/06/2024 09:30, Christoph Hellwig wrote:
Highlevel question:  in a lot of the discussions we've used the
term "untorn writes" instead, which feels better than atomic to
me as atomic is a highly overloaded term.  Should we switch the
naming to that?

I have no strong attachment to that name (atomic).

For both SCSI and NVMe, it's an "atomic" feature and I was basing the naming on that.

We could have RWF_NOTEARS or RWF_UNTEARABLE_WRITE or RWF_UNTEARABLE or RWF_UNTORN or similar. Any preference?


diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0283cf366c2a..6cb67882bcfd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -45,6 +45,7 @@
  #include <linux/slab.h>
  #include <linux/maple_tree.h>
  #include <linux/rw_hint.h>
+#include <linux/uio.h>

fs.h is included almost everywhere, so if we can avoid pulling in
even more dependencies that would be great.

It seems like it is pulled in just for this helper:

right


+static inline
+bool generic_atomic_write_valid(loff_t pos, struct iov_iter *iter)
+{
+	size_t len = iov_iter_count(iter);
+
+	if (!iter_is_ubuf(iter))
+		return false;
+
+	if (!is_power_of_2(len))
+		return false;
+
+	if (!IS_ALIGNED(pos, len))
+		return false;
+
+	return true;
+}

should that just go to uio.h instead, or move out of line?

ok, I am not sure about moving to uio.h, but I'll try to do something about this issue


Also the return type formatting is wrong, the two normal styles are
either:

static inline bool generic_atomic_write_valid(loff_t pos,
		struct iov_iter *iter)

or:

static inline bool
generic_atomic_write_valid(loff_t pos, struct iov_iter *iter)

(and while I'm at nitpicking, passing the pos before the iter
feels weird)

generally pos is first and then len (which iter provides) when a function accepts position and length, but then iter is the "larger" arg, and normally they go first. Anyway I don't mind changing that as you suggest.


Last but not least: if READ/WRITE is passed to kiocb_set_rw_flags,
it should probably set IOCB_WRITE as well?  That might be a worthwile
prep patch on it's own.

For io_uring/rw.c, we have io_write() -> io_rw_init_file(..., WRITE), and then later we set IOCB_WRITE, so would be neat to use there. But then do_iter_readv_writev() does not set IOCB_WRITE - I can't imagine that setting IOCB_WRITE would do any harm there. I see a similar change in https://lore.kernel.org/linux-fsdevel/167391048988.2311931.1567396746365286847.stgit@xxxxxxxxxxxxxxxxxxxxxx/

AFAICS, setting IOCB_WRITE is quite inconsistent. From browsing through fsdevel on lore, there was some history in trying to use IOCB_WRITE always instead of iov_iter direction. Any idea what happened to that?

I'm just getting the feeling that setting IOCB_WRITE in kiocb_set_rw_flags() is a small part - and maybe counter productive - of a larger job of fixing IOCB_WRITE usage.

Thanks,
John




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux