| Message ID | 163162767601.438332.9017034724960075707.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | fscache: Replace and remove old I/O API | expand |
On Tue, Sep 14, 2021 at 9:55 AM David Howells <dhowells@redhat.com> wrote: > > > Here's a set of patches that removes the old fscache I/O API by the following > means: > > (1) A simple fallback API is added that can read or write a single page > synchronously. The functions for this have "deprecated" in their names > as they have to be removed at some point. > > (2) An implementation of this is provided in cachefiles. It creates a kiocb > to use DIO to the backing file rather than calling readpage on the > backing filesystem page and then snooping the page wait queue. > > (3) NFS is switched to use the fallback API. > > (4) CIFS is switched to use the fallback API also for the moment. > > (5) 9P is switched to using netfslib. > > (6) The old I/O API is removed from fscache and the page snooping > implementation is removed from cachefiles. > > The reasons for doing this are: > > (A) Using a kiocb to do asynchronous DIO from/to the pages of the backing > file is now a possibility that didn't exist when cachefiles was created. > This is much simpler than the snooping mechanism with a proper callback > path and it also requires fewer copies and less memory. > > (B) We have to stop using bmap() or SEEK_DATA/SEEK_HOLE to work out what > blocks are present in the backing file is dangerous and can lead to data > corruption if the backing filesystem can insert or remove blocks of zeros > arbitrarily in order to optimise its extent list[1]. > > Whilst this patchset doesn't fix that yet, it does simplify the code and > the fix for that can be made in a subsequent patchset. > > (C) In order to fix (B), the cache will need to keep track itself of what > data is present. To make this easier to manage, the intention is to > increase the cache block granularity to, say, 256KiB - importantly, a > size that will span multiple pages - which means the single-page > interface will have to go away. netfslib is designed to deal with > that on behalf of a filesystem, though a filesystem could use raw > cache calls instead and manage things itself. > > These patches can be found also on: > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-iter-3 > > David > > Link: https://lore.kernel.org/r/YO17ZNOcq+9PajfQ@mit.edu [1] > --- > David Howells (8): > fscache: Generalise the ->begin_read_operation method > fscache: Implement an alternate I/O interface to replace the old API > nfs: Move to using the alternate (deprecated) fscache I/O API > 9p: (untested) Convert to using the netfs helper lib to do reads and caching > cifs: (untested) Move to using the alternate (deprecated) fscache I/O API > fscache: Remove the old I/O API > fscache: Remove stats that are no longer used > fscache: Update the documentation to reflect I/O API changes > > > .../filesystems/caching/backend-api.rst | 138 +-- > .../filesystems/caching/netfs-api.rst | 386 +----- > fs/9p/Kconfig | 1 + > fs/9p/cache.c | 137 --- > fs/9p/cache.h | 98 +- > fs/9p/v9fs.h | 9 + > fs/9p/vfs_addr.c | 174 ++- > fs/9p/vfs_file.c | 21 +- > fs/cachefiles/Makefile | 1 - > fs/cachefiles/interface.c | 15 - > fs/cachefiles/internal.h | 38 - > fs/cachefiles/io.c | 28 +- > fs/cachefiles/main.c | 1 - > fs/cachefiles/rdwr.c | 972 --------------- > fs/cifs/file.c | 64 +- > fs/cifs/fscache.c | 105 +- > fs/cifs/fscache.h | 74 +- > fs/fscache/cache.c | 6 - > fs/fscache/cookie.c | 10 - > fs/fscache/internal.h | 58 +- > fs/fscache/io.c | 140 ++- > fs/fscache/object.c | 2 - > fs/fscache/page.c | 1066 ----------------- > fs/fscache/stats.c | 73 +- > fs/nfs/file.c | 14 +- > fs/nfs/fscache-index.c | 26 - > fs/nfs/fscache.c | 161 +-- > fs/nfs/fscache.h | 84 +- > fs/nfs/read.c | 25 +- > fs/nfs/write.c | 7 +- > include/linux/fscache-cache.h | 131 -- > include/linux/fscache.h | 418 ++----- > include/linux/netfs.h | 17 +- > 33 files changed, 508 insertions(+), 3992 deletions(-) > delete mode 100644 fs/cachefiles/rdwr.c > > I tested an earlier version of these with NFS, which identified a couple issues which you fixed. Last I checked my unit tests and xfstests were looking good. I'll do some testing on this latest branch / patches and report back.
On Tue, 2021-09-14 at 14:54 +0100, David Howells wrote: > Here's a set of patches that removes the old fscache I/O API by the following > means: > > (1) A simple fallback API is added that can read or write a single page > synchronously. The functions for this have "deprecated" in their names > as they have to be removed at some point. > > (2) An implementation of this is provided in cachefiles. It creates a kiocb > to use DIO to the backing file rather than calling readpage on the > backing filesystem page and then snooping the page wait queue. > > (3) NFS is switched to use the fallback API. > > (4) CIFS is switched to use the fallback API also for the moment. > > (5) 9P is switched to using netfslib. > > (6) The old I/O API is removed from fscache and the page snooping > implementation is removed from cachefiles. > > The reasons for doing this are: > > (A) Using a kiocb to do asynchronous DIO from/to the pages of the backing > file is now a possibility that didn't exist when cachefiles was created. > This is much simpler than the snooping mechanism with a proper callback > path and it also requires fewer copies and less memory. > > (B) We have to stop using bmap() or SEEK_DATA/SEEK_HOLE to work out what > blocks are present in the backing file is dangerous and can lead to data > corruption if the backing filesystem can insert or remove blocks of zeros > arbitrarily in order to optimise its extent list[1]. > > Whilst this patchset doesn't fix that yet, it does simplify the code and > the fix for that can be made in a subsequent patchset. > > (C) In order to fix (B), the cache will need to keep track itself of what > data is present. To make this easier to manage, the intention is to > increase the cache block granularity to, say, 256KiB - importantly, a > size that will span multiple pages - which means the single-page > interface will have to go away. netfslib is designed to deal with > that on behalf of a filesystem, though a filesystem could use raw > cache calls instead and manage things itself. > > These patches can be found also on: > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-iter-3 > > David > > Link: https://lore.kernel.org/r/YO17ZNOcq+9PajfQ@mit.edu [1] > --- > David Howells (8): > fscache: Generalise the ->begin_read_operation method > fscache: Implement an alternate I/O interface to replace the old API > nfs: Move to using the alternate (deprecated) fscache I/O API > 9p: (untested) Convert to using the netfs helper lib to do reads and caching > cifs: (untested) Move to using the alternate (deprecated) fscache I/O API > fscache: Remove the old I/O API > fscache: Remove stats that are no longer used > fscache: Update the documentation to reflect I/O API changes > > > .../filesystems/caching/backend-api.rst | 138 +-- > .../filesystems/caching/netfs-api.rst | 386 +----- > fs/9p/Kconfig | 1 + > fs/9p/cache.c | 137 --- > fs/9p/cache.h | 98 +- > fs/9p/v9fs.h | 9 + > fs/9p/vfs_addr.c | 174 ++- > fs/9p/vfs_file.c | 21 +- > fs/cachefiles/Makefile | 1 - > fs/cachefiles/interface.c | 15 - > fs/cachefiles/internal.h | 38 - > fs/cachefiles/io.c | 28 +- > fs/cachefiles/main.c | 1 - > fs/cachefiles/rdwr.c | 972 --------------- > fs/cifs/file.c | 64 +- > fs/cifs/fscache.c | 105 +- > fs/cifs/fscache.h | 74 +- > fs/fscache/cache.c | 6 - > fs/fscache/cookie.c | 10 - > fs/fscache/internal.h | 58 +- > fs/fscache/io.c | 140 ++- > fs/fscache/object.c | 2 - > fs/fscache/page.c | 1066 ----------------- > fs/fscache/stats.c | 73 +- > fs/nfs/file.c | 14 +- > fs/nfs/fscache-index.c | 26 - > fs/nfs/fscache.c | 161 +-- > fs/nfs/fscache.h | 84 +- > fs/nfs/read.c | 25 +- > fs/nfs/write.c | 7 +- > include/linux/fscache-cache.h | 131 -- > include/linux/fscache.h | 418 ++----- > include/linux/netfs.h | 17 +- > 33 files changed, 508 insertions(+), 3992 deletions(-) > delete mode 100644 fs/cachefiles/rdwr.c > > Love the diffstat. I looked over the set and didn't see any problems with it. I also ran a quick test with ceph+fscache and didn't run into any issues, but that's not too surprising since this patchset doesn't really change the codepaths that it uses. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Tue, Sep 14, 2021 at 6:54 AM David Howells <dhowells@redhat.com> wrote: > > (1) A simple fallback API is added that can read or write a single page > synchronously. The functions for this have "deprecated" in their names > as they have to be removed at some point. I'm looking at those patches, and there's no way I'll apply anything that starts out with moving to a "deprecated" interface. Call it "fallback" or "simple" or something that shows the intent, but no, I'm not taking patches that introduce a _new_ interface and call it "deprecated". Linus
On Tue, Sep 14, 2021 at 9:21 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Call it "fallback" or "simple" or something that shows the intent, but > no, I'm not taking patches that introduce a _new_ interface and call > it "deprecated". Put another way: to call something "deprecated", you have to already have the replacement all ready to go. And if you have that, then converting existing code to a deprecated model isn't the way to go. So in neither situation does it make any sense to convert anything to a model that is deprecated. Linus
Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Call it "fallback" or "simple" or something that shows the intent, but > > no, I'm not taking patches that introduce a _new_ interface and call > > it "deprecated". Yeah, I'll change it to "fallback" - I started talking about it like that in the docs anyway. > Put another way: to call something "deprecated", you have to already > have the replacement all ready to go. We're not far off. There's a fair distance (in number of patches) between this patchset and the completion, hence why I marked them as deprecated here, intending to remove them at the end. Between myself, Jeff and Dave we have fscache, cachefiles, afs, ceph and nfs (almost) covered. I have patches for 9p and I've given a partial patch for cifs to Steve and Shyam. David
On Tue, Sep 14, 2021 at 11:30 AM David Wysochanski <dwysocha@redhat.com> wrote: > > On Tue, Sep 14, 2021 at 9:55 AM David Howells <dhowells@redhat.com> wrote: > > > > > > Here's a set of patches that removes the old fscache I/O API by the following > > means: > > > > (1) A simple fallback API is added that can read or write a single page > > synchronously. The functions for this have "deprecated" in their names > > as they have to be removed at some point. > > > > (2) An implementation of this is provided in cachefiles. It creates a kiocb > > to use DIO to the backing file rather than calling readpage on the > > backing filesystem page and then snooping the page wait queue. > > > > (3) NFS is switched to use the fallback API. > > > > (4) CIFS is switched to use the fallback API also for the moment. > > > > (5) 9P is switched to using netfslib. > > > > (6) The old I/O API is removed from fscache and the page snooping > > implementation is removed from cachefiles. > > > > The reasons for doing this are: > > > > (A) Using a kiocb to do asynchronous DIO from/to the pages of the backing > > file is now a possibility that didn't exist when cachefiles was created. > > This is much simpler than the snooping mechanism with a proper callback > > path and it also requires fewer copies and less memory. > > > > (B) We have to stop using bmap() or SEEK_DATA/SEEK_HOLE to work out what > > blocks are present in the backing file is dangerous and can lead to data > > corruption if the backing filesystem can insert or remove blocks of zeros > > arbitrarily in order to optimise its extent list[1]. > > > > Whilst this patchset doesn't fix that yet, it does simplify the code and > > the fix for that can be made in a subsequent patchset. > > > > (C) In order to fix (B), the cache will need to keep track itself of what > > data is present. To make this easier to manage, the intention is to > > increase the cache block granularity to, say, 256KiB - importantly, a > > size that will span multiple pages - which means the single-page > > interface will have to go away. netfslib is designed to deal with > > that on behalf of a filesystem, though a filesystem could use raw > > cache calls instead and manage things itself. > > > > These patches can be found also on: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-iter-3 > > > > David > > > > Link: https://lore.kernel.org/r/YO17ZNOcq+9PajfQ@mit.edu [1] > > --- > > David Howells (8): > > fscache: Generalise the ->begin_read_operation method > > fscache: Implement an alternate I/O interface to replace the old API > > nfs: Move to using the alternate (deprecated) fscache I/O API > > 9p: (untested) Convert to using the netfs helper lib to do reads and caching > > cifs: (untested) Move to using the alternate (deprecated) fscache I/O API > > fscache: Remove the old I/O API > > fscache: Remove stats that are no longer used > > fscache: Update the documentation to reflect I/O API changes > > > > > > .../filesystems/caching/backend-api.rst | 138 +-- > > .../filesystems/caching/netfs-api.rst | 386 +----- > > fs/9p/Kconfig | 1 + > > fs/9p/cache.c | 137 --- > > fs/9p/cache.h | 98 +- > > fs/9p/v9fs.h | 9 + > > fs/9p/vfs_addr.c | 174 ++- > > fs/9p/vfs_file.c | 21 +- > > fs/cachefiles/Makefile | 1 - > > fs/cachefiles/interface.c | 15 - > > fs/cachefiles/internal.h | 38 - > > fs/cachefiles/io.c | 28 +- > > fs/cachefiles/main.c | 1 - > > fs/cachefiles/rdwr.c | 972 --------------- > > fs/cifs/file.c | 64 +- > > fs/cifs/fscache.c | 105 +- > > fs/cifs/fscache.h | 74 +- > > fs/fscache/cache.c | 6 - > > fs/fscache/cookie.c | 10 - > > fs/fscache/internal.h | 58 +- > > fs/fscache/io.c | 140 ++- > > fs/fscache/object.c | 2 - > > fs/fscache/page.c | 1066 ----------------- > > fs/fscache/stats.c | 73 +- > > fs/nfs/file.c | 14 +- > > fs/nfs/fscache-index.c | 26 - > > fs/nfs/fscache.c | 161 +-- > > fs/nfs/fscache.h | 84 +- > > fs/nfs/read.c | 25 +- > > fs/nfs/write.c | 7 +- > > include/linux/fscache-cache.h | 131 -- > > include/linux/fscache.h | 418 ++----- > > include/linux/netfs.h | 17 +- > > 33 files changed, 508 insertions(+), 3992 deletions(-) > > delete mode 100644 fs/cachefiles/rdwr.c > > > > > > I tested an earlier version of these with NFS, which identified a > couple issues which you fixed. Last I checked my unit tests and > xfstests were looking good. I'll do some testing on this latest branch > / patches and report back. For the series, you can add Tested-by: Dave Wysochanski <dwysocha@redhat.com> Testing was limited to NFS enabled code paths. I ran custom unit tests, as well as a series of xfstest generic runs with various NFS versions, both fscache enabled and not enabled, as well as various NFS servers, comparing 5.15.0-rc1 runs vs runs with these patches. I did not see any failures with these new patches that were not already present with 5.15.0-rc1. Here are the list of xfstest generic runs: 1. Hammerspace (pNFS flexfiles) version 4.6.3-166: vers=4.1,fsc 2. Hammerspace (pNFS flexfiles) version 4.6.3-166: vers=4.2 3. Netapp (pNFS filelayout) version 9.5RC1: vers=4.1 4. Netapp (pNFS filelayout) version 9.5RC1: vers=4.1,fsc 5. Red Hat version 8.2 (kernel-4.18.0-193.el8): vers=4.2,fsc 6. Red Hat version 8.2 (kernel-4.18.0-193.el8): vers=4.0,fsc 7. Red Hat version 8.2 (kernel-4.18.0-193.el8): vers=3,fsc
Here's a set of patches that removes the old fscache I/O API by the following means: (1) A simple fallback API is added that can read or write a single page synchronously. The functions for this have "deprecated" in their names as they have to be removed at some point. (2) An implementation of this is provided in cachefiles. It creates a kiocb to use DIO to the backing file rather than calling readpage on the backing filesystem page and then snooping the page wait queue. (3) NFS is switched to use the fallback API. (4) CIFS is switched to use the fallback API also for the moment. (5) 9P is switched to using netfslib. (6) The old I/O API is removed from fscache and the page snooping implementation is removed from cachefiles. The reasons for doing this are: (A) Using a kiocb to do asynchronous DIO from/to the pages of the backing file is now a possibility that didn't exist when cachefiles was created. This is much simpler than the snooping mechanism with a proper callback path and it also requires fewer copies and less memory. (B) We have to stop using bmap() or SEEK_DATA/SEEK_HOLE to work out what blocks are present in the backing file is dangerous and can lead to data corruption if the backing filesystem can insert or remove blocks of zeros arbitrarily in order to optimise its extent list[1]. Whilst this patchset doesn't fix that yet, it does simplify the code and the fix for that can be made in a subsequent patchset. (C) In order to fix (B), the cache will need to keep track itself of what data is present. To make this easier to manage, the intention is to increase the cache block granularity to, say, 256KiB - importantly, a size that will span multiple pages - which means the single-page interface will have to go away. netfslib is designed to deal with that on behalf of a filesystem, though a filesystem could use raw cache calls instead and manage things itself. These patches can be found also on: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-iter-3 David Link: https://lore.kernel.org/r/YO17ZNOcq+9PajfQ@mit.edu [1] --- David Howells (8): fscache: Generalise the ->begin_read_operation method fscache: Implement an alternate I/O interface to replace the old API nfs: Move to using the alternate (deprecated) fscache I/O API 9p: (untested) Convert to using the netfs helper lib to do reads and caching cifs: (untested) Move to using the alternate (deprecated) fscache I/O API fscache: Remove the old I/O API fscache: Remove stats that are no longer used fscache: Update the documentation to reflect I/O API changes .../filesystems/caching/backend-api.rst | 138 +-- .../filesystems/caching/netfs-api.rst | 386 +----- fs/9p/Kconfig | 1 + fs/9p/cache.c | 137 --- fs/9p/cache.h | 98 +- fs/9p/v9fs.h | 9 + fs/9p/vfs_addr.c | 174 ++- fs/9p/vfs_file.c | 21 +- fs/cachefiles/Makefile | 1 - fs/cachefiles/interface.c | 15 - fs/cachefiles/internal.h | 38 - fs/cachefiles/io.c | 28 +- fs/cachefiles/main.c | 1 - fs/cachefiles/rdwr.c | 972 --------------- fs/cifs/file.c | 64 +- fs/cifs/fscache.c | 105 +- fs/cifs/fscache.h | 74 +- fs/fscache/cache.c | 6 - fs/fscache/cookie.c | 10 - fs/fscache/internal.h | 58 +- fs/fscache/io.c | 140 ++- fs/fscache/object.c | 2 - fs/fscache/page.c | 1066 ----------------- fs/fscache/stats.c | 73 +- fs/nfs/file.c | 14 +- fs/nfs/fscache-index.c | 26 - fs/nfs/fscache.c | 161 +-- fs/nfs/fscache.h | 84 +- fs/nfs/read.c | 25 +- fs/nfs/write.c | 7 +- include/linux/fscache-cache.h | 131 -- include/linux/fscache.h | 418 ++----- include/linux/netfs.h | 17 +- 33 files changed, 508 insertions(+), 3992 deletions(-) delete mode 100644 fs/cachefiles/rdwr.c