| Message ID | 20220504162342.573651-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] iomap: allow the file system to provide a bio_set for direct I/O | expand |
On 4.05.22 г. 19:23 ч., Christoph Hellwig wrote: > Allow the file system to keep state for all iterations. For now only > wire it up for direct I/O as there is an immediate need for it there. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/direct-io.c | 8 ++++++++ > include/linux/iomap.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c > index 15929690d89e3..355abe2eacc6a 100644 > --- a/fs/iomap/direct-io.c > +++ b/fs/iomap/direct-io.c > @@ -520,6 +520,14 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > dio->submit.waiter = current; > dio->submit.poll_bio = NULL; > > + /* > + * Transfer the private data that was passed by the caller to the > + * iomap_iter, and clear it in the iocb, as iocb->private will be > + * used for polled bio completion later. > + */ > + iomi.private = iocb->private; > + WRITE_ONCE(iocb->private, NULL); nit: Why use WRITE_ONCE here? Generaly when it's used it will suggest to the reader something funny is going on with accessing that variable without holding a particular lock? > + > if (iov_iter_rw(iter) == READ) { > if (iomi.pos >= dio->i_size) > goto out_free_dio; > diff --git a/include/linux/iomap.h b/include/linux/iomap.h > index a5483020dad41..109c055865f73 100644 > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -188,6 +188,7 @@ struct iomap_iter { > unsigned flags; > struct iomap iomap; > struct iomap srcmap; > + void *private; > }; > > int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
On Thu, May 05, 2022 at 11:06:50AM +0300, Nikolay Borisov wrote: >> @@ -520,6 +520,14 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, >> dio->submit.waiter = current; >> dio->submit.poll_bio = NULL; >> + /* >> + * Transfer the private data that was passed by the caller to the >> + * iomap_iter, and clear it in the iocb, as iocb->private will be >> + * used for polled bio completion later. >> + */ >> + iomi.private = iocb->private; >> + WRITE_ONCE(iocb->private, NULL); > > nit: Why use WRITE_ONCE here? Generaly when it's used it will suggest to > the reader something funny is going on with accessing that variable without > holding a particular lock? Because we use WRITE_ONCE on iocb->private later on when we use it to store the bio that is polled for, and we really want the store that clears it to NULL to be done before we start dealing with bio submission.
On Wed, May 04, 2022 at 09:23:39AM -0700, Christoph Hellwig wrote: > Allow the file system to keep state for all iterations. For now only > wire it up for direct I/O as there is an immediate need for it there. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/direct-io.c | 8 ++++++++ > include/linux/iomap.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c > index 15929690d89e3..355abe2eacc6a 100644 > --- a/fs/iomap/direct-io.c > +++ b/fs/iomap/direct-io.c > @@ -520,6 +520,14 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > dio->submit.waiter = current; > dio->submit.poll_bio = NULL; > > + /* > + * Transfer the private data that was passed by the caller to the > + * iomap_iter, and clear it in the iocb, as iocb->private will be > + * used for polled bio completion later. > + */ > + iomi.private = iocb->private; > + WRITE_ONCE(iocb->private, NULL); Do we need to transfer it back after the bio completes? Or is it a feature that iocb->private changes to the bio? --D > + > if (iov_iter_rw(iter) == READ) { > if (iomi.pos >= dio->i_size) > goto out_free_dio; > diff --git a/include/linux/iomap.h b/include/linux/iomap.h > index a5483020dad41..109c055865f73 100644 > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -188,6 +188,7 @@ struct iomap_iter { > unsigned flags; > struct iomap iomap; > struct iomap srcmap; > + void *private; > }; > > int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops); > -- > 2.30.2 >
On Thu, May 05, 2022 at 08:41:26AM -0700, Darrick J. Wong wrote: > > + */ > > + iomi.private = iocb->private; > > + WRITE_ONCE(iocb->private, NULL); > > Do we need to transfer it back after the bio completes? Or is it a > feature that iocb->private changes to the bio? No need to transfer it back. It ist just a creative way to pass private data in. Initially I just added yet another argument to iomap_dio_rw, and maybe I should just go back to that to make the things easier to follow.
On Thu, May 05, 2022 at 05:45:57PM +0200, Christoph Hellwig wrote: > On Thu, May 05, 2022 at 08:41:26AM -0700, Darrick J. Wong wrote: > > > + */ > > > + iomi.private = iocb->private; > > > + WRITE_ONCE(iocb->private, NULL); > > > > Do we need to transfer it back after the bio completes? Or is it a > > feature that iocb->private changes to the bio? > > No need to transfer it back. It ist just a creative way to pass private > data in. Initially I just added yet another argument to iomap_dio_rw, > and maybe I should just go back to that to make the things easier to > follow. Hmm. Who owns iocb->private? AFAICT there are two users of it -- the directio code uses it to store bios for polling; and then there's ocfs2, which apparently uses it for iocb lock state(!) flags. Getting back to iomap, I think the comment before __iomap_dio_rw should state that iocb->private will be transferred to iter->private to make that relationship more obvious, in case ocfs2 ever stumbles into iomap and explodes on impact. --D
On Thu, May 05, 2022 at 09:32:19AM -0700, Darrick J. Wong wrote: > > No need to transfer it back. It ist just a creative way to pass private > > data in. Initially I just added yet another argument to iomap_dio_rw, > > and maybe I should just go back to that to make the things easier to > > follow. > > Hmm. Who owns iocb->private? AFAICT there are two users of it -- the > directio code uses it to store bios for polling; and then there's ocfs2, > which apparently uses it for iocb lock state(!) flags. Yeah. > Getting back to iomap, I think the comment before __iomap_dio_rw should > state that iocb->private will be transferred to iter->private to make > that relationship more obvious, in case ocfs2 ever stumbles into iomap > and explodes on impact. I think I'll just look into passing an extra argument instead. It is pretty clear that using iocb->private was a little too clever and takes experienced file system developers way too much time to understand.
On Thu, May 05, 2022 at 08:15:43PM +0200, Christoph Hellwig wrote: > On Thu, May 05, 2022 at 09:32:19AM -0700, Darrick J. Wong wrote: > > > No need to transfer it back. It ist just a creative way to pass private > > > data in. Initially I just added yet another argument to iomap_dio_rw, > > > and maybe I should just go back to that to make the things easier to > > > follow. > > > > Hmm. Who owns iocb->private? AFAICT there are two users of it -- the > > directio code uses it to store bios for polling; and then there's ocfs2, > > which apparently uses it for iocb lock state(!) flags. > > Yeah. > > > Getting back to iomap, I think the comment before __iomap_dio_rw should > > state that iocb->private will be transferred to iter->private to make > > that relationship more obvious, in case ocfs2 ever stumbles into iomap > > and explodes on impact. > > I think I'll just look into passing an extra argument instead. It > is pretty clear that using iocb->private was a little too clever and > takes experienced file system developers way too much time to understand. Ok. --D
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index 15929690d89e3..355abe2eacc6a 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -520,6 +520,14 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, dio->submit.waiter = current; dio->submit.poll_bio = NULL; + /* + * Transfer the private data that was passed by the caller to the + * iomap_iter, and clear it in the iocb, as iocb->private will be + * used for polled bio completion later. + */ + iomi.private = iocb->private; + WRITE_ONCE(iocb->private, NULL); + if (iov_iter_rw(iter) == READ) { if (iomi.pos >= dio->i_size) goto out_free_dio; diff --git a/include/linux/iomap.h b/include/linux/iomap.h index a5483020dad41..109c055865f73 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -188,6 +188,7 @@ struct iomap_iter { unsigned flags; struct iomap iomap; struct iomap srcmap; + void *private; }; int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
Allow the file system to keep state for all iterations. For now only wire it up for direct I/O as there is an immediate need for it there. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/direct-io.c | 8 ++++++++ include/linux/iomap.h | 1 + 2 files changed, 9 insertions(+)