That is a NACK for me.
Your change inverts lock ordering so that after your change we hold the
inode lock on the coda inode before we calls file_start_write.
See the comments for sb_start_write in include/linux/fs.h
(__sb_start_write is pretty much the only thing file_start_write calls).
* Since freeze protection behaves as a lock, users have to preserve
* ordering of freeze protection and other filesystem locks. Generally,
* freeze protection should be the outermost lock. In particular, we
* have:
*
* sb_start_write
* -> i_mutex (write path, truncate, directory ops,
* ...)
* -> s_umount (freeze_super, thaw_super)
Jan
On Tue, Nov 14, 2023 at 05:32:48PM +0200, Amir Goldstein wrote:
...
> diff --git a/fs/coda/file.c b/fs/coda/file.c
> index 16acc58311ea..7c84555c8923 100644
> --- a/fs/coda/file.c
> +++ b/fs/coda/file.c
> @@ -79,14 +79,12 @@ coda_file_write_iter(struct kiocb *iocb, struct iov_iter *to)
> if (ret)
> goto finish_write;
>
> - file_start_write(host_file);
> inode_lock(coda_inode);
> - ret = vfs_iter_write(cfi->cfi_container, to, &iocb->ki_pos, 0);
> + ret = vfs_iter_write(host_file, to, &iocb->ki_pos, 0);
> coda_inode->i_size = file_inode(host_file)->i_size;
> coda_inode->i_blocks = (coda_inode->i_size + 511) >> 9;
> inode_set_mtime_to_ts(coda_inode, inode_set_ctime_current(coda_inode));
> inode_unlock(coda_inode);
> - file_end_write(host_file);
>
> finish_write:
> venus_access_intent(coda_inode->i_sb, coda_i2f(coda_inode),
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 5d704461e3b4..35c9546b3396 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1186,9 +1186,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
> since = READ_ONCE(file->f_wb_err);
> if (verf)
> nfsd_copy_write_verifier(verf, nn);
> - file_start_write(file);
> host_err = vfs_iter_write(file, &iter, &pos, flags);
> - file_end_write(file);
> if (host_err < 0) {
> commit_reset_write_verifier(nn, rqstp, host_err);
> goto out_nfserr;
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 131621daeb13..690b173f34fc 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -436,9 +436,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
> if (is_sync_kiocb(iocb)) {
> rwf_t rwf = iocb_to_rw_flags(ifl);
>
> - file_start_write(real.file);
> ret = vfs_iter_write(real.file, iter, &iocb->ki_pos, rwf);
> - file_end_write(real.file);
> /* Update size */
> ovl_file_modified(file);
> } else {
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 590ab228fa98..8cdc6e6a9639 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -846,7 +846,7 @@ ssize_t vfs_iter_read(struct file *file, struct iov_iter *iter, loff_t *ppos,
> EXPORT_SYMBOL(vfs_iter_read);
>
> static ssize_t do_iter_write(struct file *file, struct iov_iter *iter,
> - loff_t *pos, rwf_t flags)
> + loff_t *pos, rwf_t flags)
> {
> size_t tot_len;
> ssize_t ret = 0;
> @@ -901,11 +901,18 @@ ssize_t vfs_iocb_iter_write(struct file *file, struct kiocb *iocb,
> EXPORT_SYMBOL(vfs_iocb_iter_write);
>
> ssize_t vfs_iter_write(struct file *file, struct iov_iter *iter, loff_t *ppos,
> - rwf_t flags)
> + rwf_t flags)
> {
> + int ret;
> +
> if (!file->f_op->write_iter)
> return -EINVAL;
> - return do_iter_write(file, iter, ppos, flags);
> +
> + file_start_write(file);
> + ret = do_iter_write(file, iter, ppos, flags);
> + file_end_write(file);
> +
> + return ret;
> }
> EXPORT_SYMBOL(vfs_iter_write);
>
> --
> 2.34.1
>
>
