Re: [PATCH v3 5/6] ext4: introduce direct IO write path using iomap infrastructure
From: Christoph Hellwig <hch@infradead.org>
Date: 2019-09-16 12:12:53
Also in:
linux-fsdevel
On Thu, Sep 12, 2019 at 09:04:46PM +1000, Matthew Bobrowski wrote:
quoted hunk ↗ jump to hunk
@@ -213,12 +214,16 @@ static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from) struct inode *inode = file_inode(iocb->ki_filp); ssize_t ret; + if (unlikely(IS_IMMUTABLE(inode))) + return -EPERM; + ret = generic_write_checks(iocb, from); if (ret <= 0) return ret; - if (unlikely(IS_IMMUTABLE(inode))) - return -EPERM; + ret = file_modified(iocb->ki_filp); + if (ret) + return 0; /* * If we have encountered a bitmap-format file, the size limit
Independent of the error return issue you probably want to split modifying ext4_write_checks into a separate preparation patch.
+/*
+ * For a write that extends the inode size, ext4_dio_write_iter() will
+ * wait for the write to complete. Consequently, operations performed
+ * within this function are still covered by the inode_lock(). On
+ * success, this function returns 0.
+ */
+static int ext4_dio_write_end_io(struct kiocb *iocb, ssize_t size, int error,
+ unsigned int flags)
+{
+ int ret;
+ loff_t offset = iocb->ki_pos;
+ struct inode *inode = file_inode(iocb->ki_filp);
+
+ if (error) {
+ ret = ext4_handle_failed_inode_extension(inode, offset + size);
+ return ret ? ret : error;
+ }Just a personal opinion, but I find the use of the ternary operator here a little weird. A plain old: ret = ext4_handle_failed_inode_extension(inode, offset + size); if (ret) return ret; return error; flow much easier.
+ if (!inode_trylock(inode)) {
+ if (iocb->ki_flags & IOCB_NOWAIT)
+ return -EAGAIN;
+ inode_lock(inode);
+ }
+
+ if (!ext4_dio_checks(inode)) {
+ inode_unlock(inode);
+ /*
+ * Fallback to buffered IO if the operation on the
+ * inode is not supported by direct IO.
+ */
+ return ext4_buffered_write_iter(iocb, from);I think you want to lift the locking into the caller of this function so that you don't have to unlock and relock for the buffered write fallback.
+ if (offset + count > i_size_read(inode) ||
+ offset + count > EXT4_I(inode)->i_disksize) {
+ ext4_update_i_disksize(inode, inode->i_size);
+ extend = true;Doesn't the ext4_update_i_disksize need to be under an open journal handle?