On Thu, Sep 12, 2019 at 09:04:46PM +1000, Matthew Bobrowski wrote:
> @@ -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?
