Messages in this thread | | | Date | Mon, 26 Feb 2024 08:51:18 +0000 | Subject | Re: [PATCH v4 03/11] fs: Initial atomic write support | From | John Garry <> |
| |
..
>> >> Helper function atomic_write_valid() can be used by FSes to verify >> compliant writes. >> >> Signed-off-by: Prasad Singamsetty <prasad.singamsetty@oracle.com> >> #jpg: merge into single patch and much rewrite > > ^^^ this might be a miss I guess.
I'm not sure what you mean. Here I am just briefly commenting on much changes which I made.
> >> Signed-off-by: John Garry <john.g.garry@oracle.com> >> --- >> fs/aio.c | 8 ++++---- >> fs/btrfs/ioctl.c | 2 +- >> fs/read_write.c | 2 +- >> include/linux/fs.h | 36 +++++++++++++++++++++++++++++++++++- >> include/uapi/linux/fs.h | 5 ++++- >> io_uring/rw.c | 4 ++-- >> 6 files changed, 47 insertions(+), 10 deletions(-) >> >> diff --git a/fs/aio.c b/fs/aio.c >> index bb2ff48991f3..21bcbc076fd0 100644 >> --- a/fs/aio.c >> +++ b/fs/aio.c >> @@ -1502,7 +1502,7 @@ static void aio_complete_rw(struct kiocb *kiocb, long res) >> iocb_put(iocb); >> } >> >> -static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb) >> +static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb, int type) > > maybe rw_type?
ok
> >> { >> int ret; >> >> @@ -1528,7 +1528,7 @@ static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb) >> } else
..
>> + >> /* 32bit hashes as llseek() offset (for directories) */ >> #define FMODE_32BITHASH ((__force fmode_t)0x200) >> /* 64bit hashes as llseek() offset (for directories) */ >> @@ -328,6 +333,7 @@ enum rw_hint { >> #define IOCB_SYNC (__force int) RWF_SYNC >> #define IOCB_NOWAIT (__force int) RWF_NOWAIT >> #define IOCB_APPEND (__force int) RWF_APPEND >> +#define IOCB_ATOMIC (__force int) RWF_ATOMIC >> > > You might also want to add this definition in here too > > #define TRACE_IOCB_STRINGS \ > <...> > <...> > { IOCB_ATOMIC, "ATOMIC" }
ok
I suppose that new flag RWF_NOAPPEND in linux-next also should have this
>> >> +static inline bool atomic_write_valid(loff_t pos, struct iov_iter *iter, >> + unsigned int unit_min, unsigned int unit_max) >> +{ >> + size_t len = iov_iter_count(iter); >> + >> + if (!iter_is_ubuf(iter)) >> + return false; > > There is no mention about this limitation in the commit message of this > patch. Maybe it will be good to capture why this limitation to only > support ubuf and/or any plans to lift this restriction in future > in the commit message?
ok, I can mention this in the commit message.
> > >> + >> + if (len == unit_min || len == unit_max) { >> + /* ok if exactly min or max */ >> + } else if (len < unit_min || len > unit_max) { >> + return false; >> + } else if (!is_power_of_2(len)) { >> + return false; >> + } > > Checking for len == unit_min || len == unit_max is redundant when > unit_min and unit_max are already power of 2.
Sure, but it was an optimization, considering that typically we will be issuing unit_max in anticipated FS scenario.
Anyway, I will be changing this according to an earlier comment.
Thanks, John
| |