| Message ID | 20230522135018.2742245-26-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 5/22/23 22:50, David Howells wrote: > Provide a splice_read wrapper for zonefs. This does some checks before > proceeding and locks the inode across the call to filemap_splice_read() and > a size check in case of truncation. Splicing from direct I/O is handled by > the caller. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Christoph Hellwig <hch@lst.de> > cc: Al Viro <viro@zeniv.linux.org.uk> > cc: Jens Axboe <axboe@kernel.dk> > cc: Darrick J. Wong <djwong@kernel.org> > cc: linux-xfs@vger.kernel.org > cc: linux-fsdevel@vger.kernel.org > cc: linux-block@vger.kernel.org > cc: linux-mm@kvack.org One comment below but otherwise looks OK. Acked-by: Damien Le Moal <dlemoal@kernel.org> > --- > fs/zonefs/file.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/fs/zonefs/file.c b/fs/zonefs/file.c > index 132f01d3461f..65d4c4fe6364 100644 > --- a/fs/zonefs/file.c > +++ b/fs/zonefs/file.c > @@ -752,6 +752,44 @@ static ssize_t zonefs_file_read_iter(struct kiocb *iocb, struct iov_iter *to) > return ret; > } > > +static ssize_t zonefs_file_splice_read(struct file *in, loff_t *ppos, > + struct pipe_inode_info *pipe, > + size_t len, unsigned int flags) > +{ > + struct inode *inode = file_inode(in); > + struct zonefs_inode_info *zi = ZONEFS_I(inode); > + struct zonefs_zone *z = zonefs_inode_zone(inode); > + loff_t isize; > + ssize_t ret = 0; > + > + /* Offline zones cannot be read */ > + if (unlikely(IS_IMMUTABLE(inode) && !(inode->i_mode & 0777))) > + return -EPERM; > + > + if (*ppos >= z->z_capacity) > + return 0; > + > + inode_lock_shared(inode); > + > + /* Limit read operations to written data */ > + mutex_lock(&zi->i_truncate_mutex); > + isize = i_size_read(inode); > + if (*ppos >= isize) > + len = 0; > + else > + len = min_t(loff_t, len, isize - *ppos); > + mutex_unlock(&zi->i_truncate_mutex); > + > + if (len > 0) { > + ret = filemap_splice_read(in, ppos, pipe, len, flags); > + if (ret == -EIO) Is -EIO the only error that filemap_splice_read() may return ? There are other IO error codes that we could get from the block layer, e.g. -ETIMEDOUT etc. So "if (ret < 0)" may be better here ? > + zonefs_io_error(inode, false); > + } > + > + inode_unlock_shared(inode); > + return ret; > +} > + > /* > * Write open accounting is done only for sequential files. > */ > @@ -896,7 +934,7 @@ const struct file_operations zonefs_file_operations = { > .llseek = zonefs_file_llseek, > .read_iter = zonefs_file_read_iter, > .write_iter = zonefs_file_write_iter, > - .splice_read = generic_file_splice_read, > + .splice_read = zonefs_file_splice_read, > .splice_write = iter_file_splice_write, > .iopoll = iocb_bio_iopoll, > }; >
Damien Le Moal <dlemoal@kernel.org> wrote: > > + if (len > 0) { > > + ret = filemap_splice_read(in, ppos, pipe, len, flags); > > + if (ret == -EIO) > > Is -EIO the only error that filemap_splice_read() may return ? There are other > IO error codes that we could get from the block layer, e.g. -ETIMEDOUT etc. So > "if (ret < 0)" may be better here ? It can return -ENOMEM, -EINTR and -EAGAIN at least, none of which really count as I/O errors. I based the splice function on what zonefs_file_read_iter() does: } else { ret = generic_file_read_iter(iocb, to); if (ret == -EIO) zonefs_io_error(inode, false); } David
On 5/24/23 05:43, David Howells wrote: > Damien Le Moal <dlemoal@kernel.org> wrote: > >>> + if (len > 0) { >>> + ret = filemap_splice_read(in, ppos, pipe, len, flags); >>> + if (ret == -EIO) >> >> Is -EIO the only error that filemap_splice_read() may return ? There are other >> IO error codes that we could get from the block layer, e.g. -ETIMEDOUT etc. So >> "if (ret < 0)" may be better here ? > > It can return -ENOMEM, -EINTR and -EAGAIN at least, none of which really count > as I/O errors. I based the splice function on what zonefs_file_read_iter() > does: > > } else { > ret = generic_file_read_iter(iocb, to); > if (ret == -EIO) > zonefs_io_error(inode, false); > } Fair point. But checking again zonefs_io_error(), it will do nothing is nothing bad is detected for the zone that was used for the failed IO. So calling zonefs_io_error() for all error codes is actually fine, and likely much safer. I will change that in zonefs_file_read_iter(). Please use "if (ret < 0)" in your patch.
diff --git a/fs/zonefs/file.c b/fs/zonefs/file.c index 132f01d3461f..65d4c4fe6364 100644 --- a/fs/zonefs/file.c +++ b/fs/zonefs/file.c @@ -752,6 +752,44 @@ static ssize_t zonefs_file_read_iter(struct kiocb *iocb, struct iov_iter *to) return ret; } +static ssize_t zonefs_file_splice_read(struct file *in, loff_t *ppos, + struct pipe_inode_info *pipe, + size_t len, unsigned int flags) +{ + struct inode *inode = file_inode(in); + struct zonefs_inode_info *zi = ZONEFS_I(inode); + struct zonefs_zone *z = zonefs_inode_zone(inode); + loff_t isize; + ssize_t ret = 0; + + /* Offline zones cannot be read */ + if (unlikely(IS_IMMUTABLE(inode) && !(inode->i_mode & 0777))) + return -EPERM; + + if (*ppos >= z->z_capacity) + return 0; + + inode_lock_shared(inode); + + /* Limit read operations to written data */ + mutex_lock(&zi->i_truncate_mutex); + isize = i_size_read(inode); + if (*ppos >= isize) + len = 0; + else + len = min_t(loff_t, len, isize - *ppos); + mutex_unlock(&zi->i_truncate_mutex); + + if (len > 0) { + ret = filemap_splice_read(in, ppos, pipe, len, flags); + if (ret == -EIO) + zonefs_io_error(inode, false); + } + + inode_unlock_shared(inode); + return ret; +} + /* * Write open accounting is done only for sequential files. */ @@ -896,7 +934,7 @@ const struct file_operations zonefs_file_operations = { .llseek = zonefs_file_llseek, .read_iter = zonefs_file_read_iter, .write_iter = zonefs_file_write_iter, - .splice_read = generic_file_splice_read, + .splice_read = zonefs_file_splice_read, .splice_write = iter_file_splice_write, .iopoll = iocb_bio_iopoll, };
Provide a splice_read wrapper for zonefs. This does some checks before proceeding and locks the inode across the call to filemap_splice_read() and a size check in case of truncation. Splicing from direct I/O is handled by the caller. Signed-off-by: David Howells <dhowells@redhat.com> cc: Christoph Hellwig <hch@lst.de> cc: Al Viro <viro@zeniv.linux.org.uk> cc: Jens Axboe <axboe@kernel.dk> cc: Darrick J. Wong <djwong@kernel.org> cc: linux-xfs@vger.kernel.org cc: linux-fsdevel@vger.kernel.org cc: linux-block@vger.kernel.org cc: linux-mm@kvack.org --- fs/zonefs/file.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)