| Message ID | 20240623053532.857496-5-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [01/10] xfs: fix freeing speculative preallocations for preallocated files | expand |
On Sun, Jun 23, 2024 at 07:34:49AM +0200, Christoph Hellwig wrote: > While ->release returns int, the only caller ignores the return value. > As we're only doing cleanup work there isn't much of a point in > return a value to start with, so just document the situation instead. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_file.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index d39d0ea522d1c2..7b91cbab80da55 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -1186,6 +1186,10 @@ xfs_dir_open( > return error; > } > > +/* > + * Don't bother propagating errors. We're just doing cleanup, and the caller > + * ignores the return value anyway. Shouldn't we drop the int return from the function declaration, then? (Is that also a cleanup that's you're working on?) --D > + */ > STATIC int > xfs_file_release( > struct inode *inode, > @@ -1193,7 +1197,6 @@ xfs_file_release( > { > struct xfs_inode *ip = XFS_I(inode); > struct xfs_mount *mp = ip->i_mount; > - int error; > > /* If this is a read-only mount, don't generate I/O */ > if (xfs_is_readonly(mp)) > @@ -1211,11 +1214,8 @@ xfs_file_release( > if (!xfs_is_shutdown(mp) && > xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED)) { > xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE); > - if (ip->i_delayed_blks > 0) { > - error = filemap_flush(inode->i_mapping); > - if (error) > - return error; > - } > + if (ip->i_delayed_blks > 0) > + filemap_flush(inode->i_mapping); > } > > /* > @@ -1249,14 +1249,14 @@ xfs_file_release( > * dirty close we will still remove the speculative > * allocation, but after that we will leave it in place. > */ > - error = xfs_free_eofblocks(ip); > - if (!error && ip->i_delayed_blks) > + xfs_free_eofblocks(ip); > + if (ip->i_delayed_blks) > xfs_iflags_set(ip, XFS_IDIRTY_RELEASE); > } > xfs_iunlock(ip, XFS_IOLOCK_EXCL); > } > > - return error; > + return 0; > } > > STATIC int > -- > 2.43.0 > >
On Mon, Jun 24, 2024 at 08:39:51AM -0700, Darrick J. Wong wrote: > > +/* > > + * Don't bother propagating errors. We're just doing cleanup, and the caller > > + * ignores the return value anyway. > > Shouldn't we drop the int return from the function declaration, then? > > (Is that also a cleanup that's you're working on?) We can't drop it without changing the f_ops->release signature and updates to the many instance of it. That would still be worthwhile project, but it's something for someone who is better at scripting than me.
On Mon, Jun 24, 2024 at 05:51:24PM +0200, Christoph Hellwig wrote: > On Mon, Jun 24, 2024 at 08:39:51AM -0700, Darrick J. Wong wrote: > > > +/* > > > + * Don't bother propagating errors. We're just doing cleanup, and the caller > > > + * ignores the return value anyway. > > > > Shouldn't we drop the int return from the function declaration, then? > > > > (Is that also a cleanup that's you're working on?) > > We can't drop it without changing the f_ops->release signature and > updates to the many instance of it. That would still be worthwhile > project, but it's something for someone who is better at scripting > than me. Yeahhhhh... at this point it's a giant treewide change that just doesn't seem worth it. Maybe save it for someone who wants to make a subtle but important behavior change and needs a type signature change to stand in as an idiot light for out of tree modules. :P Anyway I think this is fine, let's merge this series Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index d39d0ea522d1c2..7b91cbab80da55 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1186,6 +1186,10 @@ xfs_dir_open( return error; } +/* + * Don't bother propagating errors. We're just doing cleanup, and the caller + * ignores the return value anyway. + */ STATIC int xfs_file_release( struct inode *inode, @@ -1193,7 +1197,6 @@ xfs_file_release( { struct xfs_inode *ip = XFS_I(inode); struct xfs_mount *mp = ip->i_mount; - int error; /* If this is a read-only mount, don't generate I/O */ if (xfs_is_readonly(mp)) @@ -1211,11 +1214,8 @@ xfs_file_release( if (!xfs_is_shutdown(mp) && xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED)) { xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE); - if (ip->i_delayed_blks > 0) { - error = filemap_flush(inode->i_mapping); - if (error) - return error; - } + if (ip->i_delayed_blks > 0) + filemap_flush(inode->i_mapping); } /* @@ -1249,14 +1249,14 @@ xfs_file_release( * dirty close we will still remove the speculative * allocation, but after that we will leave it in place. */ - error = xfs_free_eofblocks(ip); - if (!error && ip->i_delayed_blks) + xfs_free_eofblocks(ip); + if (ip->i_delayed_blks) xfs_iflags_set(ip, XFS_IDIRTY_RELEASE); } xfs_iunlock(ip, XFS_IOLOCK_EXCL); } - return error; + return 0; } STATIC int
While ->release returns int, the only caller ignores the return value. As we're only doing cleanup work there isn't much of a point in return a value to start with, so just document the situation instead. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_file.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)