| Message ID | 20230519093521.133226-9-hch@lst.de |
|---|---|
| State | New |
| Headers | show |
| Series | [01/13] iomap: update ki_pos a little later in iomap_dio_complete | expand |
On 5/19/23 18:35, Christoph Hellwig wrote: > Move the assignment to current->backing_dev_info from the callers into > iomap_file_buffered_write to reduce boiler plate code and reduce the > scope to just around the page dirtying loop. > > Note that zonefs was missing this assignment before. Hu... Shouldn't this be fixed as a separate patch with a Fixes tag for this cycle ? I have never noticed any issues with this missing though. Not sure how an issue can be triggered with this assignment missing. Apart from that, this patch look good to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
On Fri, May 19, 2023 at 11:35:16AM +0200, Christoph Hellwig wrote: > Move the assignment to current->backing_dev_info from the callers into > iomap_file_buffered_write to reduce boiler plate code and reduce the > scope to just around the page dirtying loop. > > Note that zonefs was missing this assignment before. I'm still wondering (a) what the hell current->backing_dev_info is for, and (b) if we need it around the iomap_unshare operation. $ git grep current..backing_dev_info fs/btrfs/file.c:1148: current->backing_dev_info = inode_to_bdi(inode); fs/btrfs/file.c:1169: current->backing_dev_info = NULL; fs/btrfs/file.c:1692: current->backing_dev_info = NULL; fs/ceph/file.c:1795: current->backing_dev_info = inode_to_bdi(inode); fs/ceph/file.c:1943: current->backing_dev_info = NULL; fs/ext4/file.c:288: current->backing_dev_info = inode_to_bdi(inode); fs/ext4/file.c:290: current->backing_dev_info = NULL; fs/f2fs/file.c:4520: current->backing_dev_info = inode_to_bdi(inode); fs/f2fs/file.c:4522: current->backing_dev_info = NULL; fs/fuse/file.c:1366: current->backing_dev_info = inode_to_bdi(inode); fs/fuse/file.c:1412: current->backing_dev_info = NULL; fs/gfs2/file.c:1044: current->backing_dev_info = inode_to_bdi(inode); fs/gfs2/file.c:1048: current->backing_dev_info = NULL; fs/nfs/file.c:652: current->backing_dev_info = inode_to_bdi(inode); fs/nfs/file.c:654: current->backing_dev_info = NULL; fs/ntfs/file.c:1914: current->backing_dev_info = inode_to_bdi(vi); fs/ntfs/file.c:1918: current->backing_dev_info = NULL; fs/ntfs3/file.c:823: current->backing_dev_info = inode_to_bdi(inode); fs/ntfs3/file.c:996: current->backing_dev_info = NULL; fs/xfs/xfs_file.c:721: current->backing_dev_info = inode_to_bdi(inode); fs/xfs/xfs_file.c:756: current->backing_dev_info = NULL; mm/filemap.c:3995: current->backing_dev_info = inode_to_bdi(inode); mm/filemap.c:4056: current->backing_dev_info = NULL; AFAICT nobody uses it at all? Unless there's some bizarre user that isn't extracting it from @current? Oh, hey, new question (c) isn't this set incorrectly for xfs realtime files? --D > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/gfs2/file.c | 3 --- > fs/iomap/buffered-io.c | 3 +++ > fs/xfs/xfs_file.c | 5 ----- > 3 files changed, 3 insertions(+), 8 deletions(-) > > diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c > index 499ef174dec138..261897fcfbc495 100644 > --- a/fs/gfs2/file.c > +++ b/fs/gfs2/file.c > @@ -25,7 +25,6 @@ > #include <linux/dlm.h> > #include <linux/dlm_plock.h> > #include <linux/delay.h> > -#include <linux/backing-dev.h> > #include <linux/fileattr.h> > > #include "gfs2.h" > @@ -1041,11 +1040,9 @@ static ssize_t gfs2_file_buffered_write(struct kiocb *iocb, > goto out_unlock; > } > > - current->backing_dev_info = inode_to_bdi(inode); > pagefault_disable(); > ret = iomap_file_buffered_write(iocb, from, &gfs2_iomap_ops); > pagefault_enable(); > - current->backing_dev_info = NULL; > if (ret > 0) > written += ret; > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 550525a525c45c..b2779bd1f10611 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2010 Red Hat, Inc. > * Copyright (C) 2016-2019 Christoph Hellwig. > */ > +#include <linux/backing-dev.h> > #include <linux/module.h> > #include <linux/compiler.h> > #include <linux/fs.h> > @@ -869,8 +870,10 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i, > if (iocb->ki_flags & IOCB_NOWAIT) > iter.flags |= IOMAP_NOWAIT; > > + current->backing_dev_info = inode_to_bdi(iter.inode); > while ((ret = iomap_iter(&iter, ops)) > 0) > iter.processed = iomap_write_iter(&iter, i); > + current->backing_dev_info = NULL; > > if (unlikely(ret < 0)) > return ret; > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index bfba10e0b0f3c2..98d763cc3b114c 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -27,7 +27,6 @@ > > #include <linux/dax.h> > #include <linux/falloc.h> > -#include <linux/backing-dev.h> > #include <linux/mman.h> > #include <linux/fadvise.h> > #include <linux/mount.h> > @@ -717,9 +716,6 @@ xfs_file_buffered_write( > if (ret) > goto out; > > - /* We can write back this queue in page reclaim */ > - current->backing_dev_info = inode_to_bdi(inode); > - > trace_xfs_file_buffered_write(iocb, from); > ret = iomap_file_buffered_write(iocb, from, > &xfs_buffered_write_iomap_ops); > @@ -751,7 +747,6 @@ xfs_file_buffered_write( > goto write_retry; > } > > - current->backing_dev_info = NULL; > out: > if (iolock) > xfs_iunlock(ip, iolock); > -- > 2.39.2 >
On Mon, May 22, 2023 at 06:06:27PM -0700, Darrick J. Wong wrote: > On Fri, May 19, 2023 at 11:35:16AM +0200, Christoph Hellwig wrote: > > Move the assignment to current->backing_dev_info from the callers into > > iomap_file_buffered_write to reduce boiler plate code and reduce the > > scope to just around the page dirtying loop. > > > > Note that zonefs was missing this assignment before. > > I'm still wondering (a) what the hell current->backing_dev_info is for, > and (b) if we need it around the iomap_unshare operation. > > $ git grep current..backing_dev_info [results show it only set, never used] > > AFAICT nobody uses it at all? Unless there's some bizarre user that > isn't extracting it from @current? > > Oh, hey, new question (c) isn't this set incorrectly for xfs realtime > files? Some git archaelogy ... This was first introduced in commit 2f45a06517a62 (in the linux-fullhistory tree) in 2002 by one Andrew Morton. At the time, it added this check to the page scanner: + if (page->pte.direct || + page->mapping->backing_dev_info == + current->backing_dev_info) { + wait_on_page_writeback(page); + } AFAICT (the code went through some metamorphoses in the intervening twenty years), the last use of it ended up in current_may_throttle(), and it was removed in March 2022 by Neil Brown in commit b9b1335e6403. Since then, there have been no users of task->backing_dev_info, and I'm pretty sure it can go away.
On Tue, May 23, 2023 at 04:30:51AM +0100, Matthew Wilcox wrote: > AFAICT (the code went through some metamorphoses in the intervening > twenty years), the last use of it ended up in current_may_throttle(), > and it was removed in March 2022 by Neil Brown in commit b9b1335e6403. > Since then, there have been no users of task->backing_dev_info, and I'm > pretty sure it can go away. Oh, nice. I hadn't noticed it finally went away. The next iteration of the series will just remove it.
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 499ef174dec138..261897fcfbc495 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -25,7 +25,6 @@ #include <linux/dlm.h> #include <linux/dlm_plock.h> #include <linux/delay.h> -#include <linux/backing-dev.h> #include <linux/fileattr.h> #include "gfs2.h" @@ -1041,11 +1040,9 @@ static ssize_t gfs2_file_buffered_write(struct kiocb *iocb, goto out_unlock; } - current->backing_dev_info = inode_to_bdi(inode); pagefault_disable(); ret = iomap_file_buffered_write(iocb, from, &gfs2_iomap_ops); pagefault_enable(); - current->backing_dev_info = NULL; if (ret > 0) written += ret; diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 550525a525c45c..b2779bd1f10611 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -3,6 +3,7 @@ * Copyright (C) 2010 Red Hat, Inc. * Copyright (C) 2016-2019 Christoph Hellwig. */ +#include <linux/backing-dev.h> #include <linux/module.h> #include <linux/compiler.h> #include <linux/fs.h> @@ -869,8 +870,10 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i, if (iocb->ki_flags & IOCB_NOWAIT) iter.flags |= IOMAP_NOWAIT; + current->backing_dev_info = inode_to_bdi(iter.inode); while ((ret = iomap_iter(&iter, ops)) > 0) iter.processed = iomap_write_iter(&iter, i); + current->backing_dev_info = NULL; if (unlikely(ret < 0)) return ret; diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index bfba10e0b0f3c2..98d763cc3b114c 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -27,7 +27,6 @@ #include <linux/dax.h> #include <linux/falloc.h> -#include <linux/backing-dev.h> #include <linux/mman.h> #include <linux/fadvise.h> #include <linux/mount.h> @@ -717,9 +716,6 @@ xfs_file_buffered_write( if (ret) goto out; - /* We can write back this queue in page reclaim */ - current->backing_dev_info = inode_to_bdi(inode); - trace_xfs_file_buffered_write(iocb, from); ret = iomap_file_buffered_write(iocb, from, &xfs_buffered_write_iomap_ops); @@ -751,7 +747,6 @@ xfs_file_buffered_write( goto write_retry; } - current->backing_dev_info = NULL; out: if (iolock) xfs_iunlock(ip, iolock);
Move the assignment to current->backing_dev_info from the callers into iomap_file_buffered_write to reduce boiler plate code and reduce the scope to just around the page dirtying loop. Note that zonefs was missing this assignment before. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/gfs2/file.c | 3 --- fs/iomap/buffered-io.c | 3 +++ fs/xfs/xfs_file.c | 5 ----- 3 files changed, 3 insertions(+), 8 deletions(-)