| Message ID | 20240103084126.513354-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/15] shmem: move the shmem_mapping assert into shmem_get_folio_gfp | expand |
On Wed, Jan 03, 2024 at 08:41:16AM +0000, Christoph Hellwig wrote: > All current and pending xfile users use the xfile_obj_load > and xfile_obj_store API, so make those the actual implementation. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > .../xfs/xfs-online-fsck-design.rst | 10 +--- > fs/xfs/scrub/trace.h | 4 +- > fs/xfs/scrub/xfile.c | 54 +++++++++---------- > fs/xfs/scrub/xfile.h | 32 +---------- > 4 files changed, 30 insertions(+), 70 deletions(-) > > diff --git a/Documentation/filesystems/xfs/xfs-online-fsck-design.rst b/Documentation/filesystems/xfs/xfs-online-fsck-design.rst > index 352516feef6ffe..324d5ec921e8e5 100644 > --- a/Documentation/filesystems/xfs/xfs-online-fsck-design.rst > +++ b/Documentation/filesystems/xfs/xfs-online-fsck-design.rst > @@ -1915,19 +1915,13 @@ four of those five higher level data structures. > The fifth use case is discussed in the :ref:`realtime summary <rtsummary>` case > study. > > -The most general storage interface supported by the xfile enables the reading > -and writing of arbitrary quantities of data at arbitrary offsets in the xfile. > -This capability is provided by ``xfile_pread`` and ``xfile_pwrite`` functions, > -which behave similarly to their userspace counterparts. > XFS is very record-based, which suggests that the ability to load and store > complete records is important. > To support these cases, a pair of ``xfile_obj_load`` and ``xfile_obj_store`` > -functions are provided to read and persist objects into an xfile. > -They are internally the same as pread and pwrite, except that they treat any > -error as an out of memory error. > +functions are provided to read and persist objects into an xfile that unlike > +the pread and pwrite system calls treat any error as an out of memory error. I don't think we need to mention pread and pwrite anymore. "To support these cases, a pair of ``xfile_obj_load`` and ``xfile_obj_store`` functions are provided to read and persist objects into an xfile. An errors encountered here are treated as an out of memory error." > For online repair, squashing error conditions in this manner is an acceptable > behavior because the only reaction is to abort the operation back to userspace. > -All five xfile usecases can be serviced by these four functions. > > However, no discussion of file access idioms is complete without answering the > question, "But what about mmap?" > diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h > index ed9e044f6d603c..e6156d000fc615 100644 > --- a/fs/xfs/scrub/trace.h > +++ b/fs/xfs/scrub/trace.h > @@ -903,8 +903,8 @@ DECLARE_EVENT_CLASS(xfile_class, > DEFINE_EVENT(xfile_class, name, \ > TP_PROTO(struct xfile *xf, loff_t pos, unsigned long long bytecount), \ > TP_ARGS(xf, pos, bytecount)) > -DEFINE_XFILE_EVENT(xfile_pread); > -DEFINE_XFILE_EVENT(xfile_pwrite); > +DEFINE_XFILE_EVENT(xfile_obj_load); > +DEFINE_XFILE_EVENT(xfile_obj_store); Want to shorten the names to xfile_load and xfile_store? That's really what they're doing anyway. With those changes, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > DEFINE_XFILE_EVENT(xfile_seek_data); > DEFINE_XFILE_EVENT(xfile_get_page); > DEFINE_XFILE_EVENT(xfile_put_page); > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c > index 87654cdd5ac6f9..9e25ecf3dc2fec 100644 > --- a/fs/xfs/scrub/xfile.c > +++ b/fs/xfs/scrub/xfile.c > @@ -118,13 +118,11 @@ xfile_destroy( > } > > /* > - * Read a memory object directly from the xfile's page cache. Unlike regular > - * pread, we return -E2BIG and -EFBIG for reads that are too large or at too > - * high an offset, instead of truncating the read. Otherwise, we return > - * bytes read or an error code, like regular pread. > + * Load an object. Since we're treating this file as "memory", any error or > + * short IO is treated as a failure to allocate memory. > */ > -ssize_t > -xfile_pread( > +int > +xfile_obj_load( > struct xfile *xf, > void *buf, > size_t count, > @@ -133,16 +131,15 @@ xfile_pread( > struct inode *inode = file_inode(xf->file); > struct address_space *mapping = inode->i_mapping; > struct page *page = NULL; > - ssize_t read = 0; > unsigned int pflags; > int error = 0; > > if (count > MAX_RW_COUNT) > - return -E2BIG; > + return -ENOMEM; > if (inode->i_sb->s_maxbytes - pos < count) > - return -EFBIG; > + return -ENOMEM; > > - trace_xfile_pread(xf, pos, count); > + trace_xfile_obj_load(xf, pos, count); > > pflags = memalloc_nofs_save(); > while (count > 0) { > @@ -160,8 +157,10 @@ xfile_pread( > __GFP_NOWARN); > if (IS_ERR(page)) { > error = PTR_ERR(page); > - if (error != -ENOMEM) > + if (error != -ENOMEM) { > + error = -ENOMEM; > break; > + } > > memset(buf, 0, len); > goto advance; > @@ -185,23 +184,18 @@ xfile_pread( > count -= len; > pos += len; > buf += len; > - read += len; > } > memalloc_nofs_restore(pflags); > > - if (read > 0) > - return read; > return error; > } > > /* > - * Write a memory object directly to the xfile's page cache. Unlike regular > - * pwrite, we return -E2BIG and -EFBIG for writes that are too large or at too > - * high an offset, instead of truncating the write. Otherwise, we return > - * bytes written or an error code, like regular pwrite. > + * Store an object. Since we're treating this file as "memory", any error or > + * short IO is treated as a failure to allocate memory. > */ > -ssize_t > -xfile_pwrite( > +int > +xfile_obj_store( > struct xfile *xf, > const void *buf, > size_t count, > @@ -211,16 +205,15 @@ xfile_pwrite( > struct address_space *mapping = inode->i_mapping; > const struct address_space_operations *aops = mapping->a_ops; > struct page *page = NULL; > - ssize_t written = 0; > unsigned int pflags; > int error = 0; > > if (count > MAX_RW_COUNT) > - return -E2BIG; > + return -ENOMEM; > if (inode->i_sb->s_maxbytes - pos < count) > - return -EFBIG; > + return -ENOMEM; > > - trace_xfile_pwrite(xf, pos, count); > + trace_xfile_obj_store(xf, pos, count); > > pflags = memalloc_nofs_save(); > while (count > 0) { > @@ -239,8 +232,10 @@ xfile_pwrite( > */ > error = aops->write_begin(NULL, mapping, pos, len, &page, > &fsdata); > - if (error) > + if (error) { > + error = -ENOMEM; > break; > + } > > /* > * xfile pages must never be mapped into userspace, so we skip > @@ -259,13 +254,14 @@ xfile_pwrite( > ret = aops->write_end(NULL, mapping, pos, len, len, page, > fsdata); > if (ret < 0) { > - error = ret; > + error = -ENOMEM; > break; > } > > - written += ret; > - if (ret != len) > + if (ret != len) { > + error = -ENOMEM; > break; > + } > > count -= ret; > pos += ret; > @@ -273,8 +269,6 @@ xfile_pwrite( > } > memalloc_nofs_restore(pflags); > > - if (written > 0) > - return written; > return error; > } > > diff --git a/fs/xfs/scrub/xfile.h b/fs/xfs/scrub/xfile.h > index c602d11560d8ee..f0e11febf216a7 100644 > --- a/fs/xfs/scrub/xfile.h > +++ b/fs/xfs/scrub/xfile.h > @@ -29,38 +29,10 @@ struct xfile { > int xfile_create(const char *description, loff_t isize, struct xfile **xfilep); > void xfile_destroy(struct xfile *xf); > > -ssize_t xfile_pread(struct xfile *xf, void *buf, size_t count, loff_t pos); > -ssize_t xfile_pwrite(struct xfile *xf, const void *buf, size_t count, > +int xfile_obj_load(struct xfile *xf, void *buf, size_t count, loff_t pos); > +int xfile_obj_store(struct xfile *xf, const void *buf, size_t count, > loff_t pos); > > -/* > - * Load an object. Since we're treating this file as "memory", any error or > - * short IO is treated as a failure to allocate memory. > - */ > -static inline int > -xfile_obj_load(struct xfile *xf, void *buf, size_t count, loff_t pos) > -{ > - ssize_t ret = xfile_pread(xf, buf, count, pos); > - > - if (ret < 0 || ret != count) > - return -ENOMEM; > - return 0; > -} > - > -/* > - * Store an object. Since we're treating this file as "memory", any error or > - * short IO is treated as a failure to allocate memory. > - */ > -static inline int > -xfile_obj_store(struct xfile *xf, const void *buf, size_t count, loff_t pos) > -{ > - ssize_t ret = xfile_pwrite(xf, buf, count, pos); > - > - if (ret < 0 || ret != count) > - return -ENOMEM; > - return 0; > -} > - > loff_t xfile_seek_data(struct xfile *xf, loff_t pos); > > int xfile_get_page(struct xfile *xf, loff_t offset, unsigned int len, > -- > 2.39.2 > >
On Wed, Jan 03, 2024 at 03:48:49PM -0800, Darrick J. Wong wrote: > "To support these cases, a pair of ``xfile_obj_load`` and ``xfile_obj_store`` > functions are provided to read and persist objects into an xfile. An errors > encountered here are treated as an out of memory error." Ok. > > -DEFINE_XFILE_EVENT(xfile_pwrite); > > +DEFINE_XFILE_EVENT(xfile_obj_load); > > +DEFINE_XFILE_EVENT(xfile_obj_store); > > Want to shorten the names to xfile_load and xfile_store? That's really > what they're doing anyway. Fine with me. Just for the trace points or also for the functions? Also - returning ENOMEM for the API misuse cases (too large object, too large total size) always seemed weird to me. Is there a really strong case for it or should we go for actually useful errors for those?
On Thu, Jan 04, 2024 at 07:15:42AM +0100, Christoph Hellwig wrote: > On Wed, Jan 03, 2024 at 03:48:49PM -0800, Darrick J. Wong wrote: > > "To support these cases, a pair of ``xfile_obj_load`` and ``xfile_obj_store`` > > functions are provided to read and persist objects into an xfile. An errors > > encountered here are treated as an out of memory error." > > Ok. > > > > -DEFINE_XFILE_EVENT(xfile_pwrite); > > > +DEFINE_XFILE_EVENT(xfile_obj_load); > > > +DEFINE_XFILE_EVENT(xfile_obj_store); > > > > Want to shorten the names to xfile_load and xfile_store? That's really > > what they're doing anyway. > > Fine with me. Just for the trace points or also for the functions? Might as well do them both, I don't think anyone really depends on those exact names. I don't. :) > Also - returning ENOMEM for the API misuse cases (too large object, > too large total size) always seemed weird to me. Is there a really > strong case for it or should we go for actually useful errors for those? The errors returned by the xfile APIs can float out to userspace, so I'd rather have them all turn into: $ xfs_io -c 'scrub <fubar>' / XFS_IOC_SCRUB_METADATA: Cannot allocate memory. vs. $ xfs_io -c 'scrub <fubar>' / XFS_IOC_SCRUB_METADATA: File is too large. So that users won't think that the root directory is too big or something. --D
diff --git a/Documentation/filesystems/xfs/xfs-online-fsck-design.rst b/Documentation/filesystems/xfs/xfs-online-fsck-design.rst index 352516feef6ffe..324d5ec921e8e5 100644 --- a/Documentation/filesystems/xfs/xfs-online-fsck-design.rst +++ b/Documentation/filesystems/xfs/xfs-online-fsck-design.rst @@ -1915,19 +1915,13 @@ four of those five higher level data structures. The fifth use case is discussed in the :ref:`realtime summary <rtsummary>` case study. -The most general storage interface supported by the xfile enables the reading -and writing of arbitrary quantities of data at arbitrary offsets in the xfile. -This capability is provided by ``xfile_pread`` and ``xfile_pwrite`` functions, -which behave similarly to their userspace counterparts. XFS is very record-based, which suggests that the ability to load and store complete records is important. To support these cases, a pair of ``xfile_obj_load`` and ``xfile_obj_store`` -functions are provided to read and persist objects into an xfile. -They are internally the same as pread and pwrite, except that they treat any -error as an out of memory error. +functions are provided to read and persist objects into an xfile that unlike +the pread and pwrite system calls treat any error as an out of memory error. For online repair, squashing error conditions in this manner is an acceptable behavior because the only reaction is to abort the operation back to userspace. -All five xfile usecases can be serviced by these four functions. However, no discussion of file access idioms is complete without answering the question, "But what about mmap?" diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h index ed9e044f6d603c..e6156d000fc615 100644 --- a/fs/xfs/scrub/trace.h +++ b/fs/xfs/scrub/trace.h @@ -903,8 +903,8 @@ DECLARE_EVENT_CLASS(xfile_class, DEFINE_EVENT(xfile_class, name, \ TP_PROTO(struct xfile *xf, loff_t pos, unsigned long long bytecount), \ TP_ARGS(xf, pos, bytecount)) -DEFINE_XFILE_EVENT(xfile_pread); -DEFINE_XFILE_EVENT(xfile_pwrite); +DEFINE_XFILE_EVENT(xfile_obj_load); +DEFINE_XFILE_EVENT(xfile_obj_store); DEFINE_XFILE_EVENT(xfile_seek_data); DEFINE_XFILE_EVENT(xfile_get_page); DEFINE_XFILE_EVENT(xfile_put_page); diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c index 87654cdd5ac6f9..9e25ecf3dc2fec 100644 --- a/fs/xfs/scrub/xfile.c +++ b/fs/xfs/scrub/xfile.c @@ -118,13 +118,11 @@ xfile_destroy( } /* - * Read a memory object directly from the xfile's page cache. Unlike regular - * pread, we return -E2BIG and -EFBIG for reads that are too large or at too - * high an offset, instead of truncating the read. Otherwise, we return - * bytes read or an error code, like regular pread. + * Load an object. Since we're treating this file as "memory", any error or + * short IO is treated as a failure to allocate memory. */ -ssize_t -xfile_pread( +int +xfile_obj_load( struct xfile *xf, void *buf, size_t count, @@ -133,16 +131,15 @@ xfile_pread( struct inode *inode = file_inode(xf->file); struct address_space *mapping = inode->i_mapping; struct page *page = NULL; - ssize_t read = 0; unsigned int pflags; int error = 0; if (count > MAX_RW_COUNT) - return -E2BIG; + return -ENOMEM; if (inode->i_sb->s_maxbytes - pos < count) - return -EFBIG; + return -ENOMEM; - trace_xfile_pread(xf, pos, count); + trace_xfile_obj_load(xf, pos, count); pflags = memalloc_nofs_save(); while (count > 0) { @@ -160,8 +157,10 @@ xfile_pread( __GFP_NOWARN); if (IS_ERR(page)) { error = PTR_ERR(page); - if (error != -ENOMEM) + if (error != -ENOMEM) { + error = -ENOMEM; break; + } memset(buf, 0, len); goto advance; @@ -185,23 +184,18 @@ xfile_pread( count -= len; pos += len; buf += len; - read += len; } memalloc_nofs_restore(pflags); - if (read > 0) - return read; return error; } /* - * Write a memory object directly to the xfile's page cache. Unlike regular - * pwrite, we return -E2BIG and -EFBIG for writes that are too large or at too - * high an offset, instead of truncating the write. Otherwise, we return - * bytes written or an error code, like regular pwrite. + * Store an object. Since we're treating this file as "memory", any error or + * short IO is treated as a failure to allocate memory. */ -ssize_t -xfile_pwrite( +int +xfile_obj_store( struct xfile *xf, const void *buf, size_t count, @@ -211,16 +205,15 @@ xfile_pwrite( struct address_space *mapping = inode->i_mapping; const struct address_space_operations *aops = mapping->a_ops; struct page *page = NULL; - ssize_t written = 0; unsigned int pflags; int error = 0; if (count > MAX_RW_COUNT) - return -E2BIG; + return -ENOMEM; if (inode->i_sb->s_maxbytes - pos < count) - return -EFBIG; + return -ENOMEM; - trace_xfile_pwrite(xf, pos, count); + trace_xfile_obj_store(xf, pos, count); pflags = memalloc_nofs_save(); while (count > 0) { @@ -239,8 +232,10 @@ xfile_pwrite( */ error = aops->write_begin(NULL, mapping, pos, len, &page, &fsdata); - if (error) + if (error) { + error = -ENOMEM; break; + } /* * xfile pages must never be mapped into userspace, so we skip @@ -259,13 +254,14 @@ xfile_pwrite( ret = aops->write_end(NULL, mapping, pos, len, len, page, fsdata); if (ret < 0) { - error = ret; + error = -ENOMEM; break; } - written += ret; - if (ret != len) + if (ret != len) { + error = -ENOMEM; break; + } count -= ret; pos += ret; @@ -273,8 +269,6 @@ xfile_pwrite( } memalloc_nofs_restore(pflags); - if (written > 0) - return written; return error; } diff --git a/fs/xfs/scrub/xfile.h b/fs/xfs/scrub/xfile.h index c602d11560d8ee..f0e11febf216a7 100644 --- a/fs/xfs/scrub/xfile.h +++ b/fs/xfs/scrub/xfile.h @@ -29,38 +29,10 @@ struct xfile { int xfile_create(const char *description, loff_t isize, struct xfile **xfilep); void xfile_destroy(struct xfile *xf); -ssize_t xfile_pread(struct xfile *xf, void *buf, size_t count, loff_t pos); -ssize_t xfile_pwrite(struct xfile *xf, const void *buf, size_t count, +int xfile_obj_load(struct xfile *xf, void *buf, size_t count, loff_t pos); +int xfile_obj_store(struct xfile *xf, const void *buf, size_t count, loff_t pos); -/* - * Load an object. Since we're treating this file as "memory", any error or - * short IO is treated as a failure to allocate memory. - */ -static inline int -xfile_obj_load(struct xfile *xf, void *buf, size_t count, loff_t pos) -{ - ssize_t ret = xfile_pread(xf, buf, count, pos); - - if (ret < 0 || ret != count) - return -ENOMEM; - return 0; -} - -/* - * Store an object. Since we're treating this file as "memory", any error or - * short IO is treated as a failure to allocate memory. - */ -static inline int -xfile_obj_store(struct xfile *xf, const void *buf, size_t count, loff_t pos) -{ - ssize_t ret = xfile_pwrite(xf, buf, count, pos); - - if (ret < 0 || ret != count) - return -ENOMEM; - return 0; -} - loff_t xfile_seek_data(struct xfile *xf, loff_t pos); int xfile_get_page(struct xfile *xf, loff_t offset, unsigned int len,
All current and pending xfile users use the xfile_obj_load and xfile_obj_store API, so make those the actual implementation. Signed-off-by: Christoph Hellwig <hch@lst.de> --- .../xfs/xfs-online-fsck-design.rst | 10 +--- fs/xfs/scrub/trace.h | 4 +- fs/xfs/scrub/xfile.c | 54 +++++++++---------- fs/xfs/scrub/xfile.h | 32 +---------- 4 files changed, 30 insertions(+), 70 deletions(-)