| Message ID | 20240623053532.857496-3-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:47AM +0200, Christoph Hellwig wrote: > xfs_release is only called from xfs_file_release, which is wired up as > the f_op->release handler for regular files only. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_inode.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 38f946e3be2da3..9a9340aebe9d8a 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -1552,9 +1552,6 @@ xfs_release( > xfs_mount_t *mp = ip->i_mount; > int error = 0; > > - if (!S_ISREG(VFS_I(ip)->i_mode) || (VFS_I(ip)->i_mode == 0)) How would we encounter !i_mode regular files being released? If an open file's link count is incorrectly low, it can't get freed until after all the open file descriptors have been released, right? Or is there some other vector for this? I'm wondering if this ought to be: if (XFS_IS_CORRUPT(mp, !VFS_I(ip)->i_mode)) { xfs_inode_mark_sick(ip); return -EFSCORRUPTED; } --D > - return 0; > - > /* If this is a read-only mount, don't do this (would generate I/O) */ > if (xfs_is_readonly(mp)) > return 0; > -- > 2.43.0 > >
On Mon, Jun 24, 2024 at 08:34:59AM -0700, Darrick J. Wong wrote: > > - if (!S_ISREG(VFS_I(ip)->i_mode) || (VFS_I(ip)->i_mode == 0)) > > How would we encounter !i_mode regular files being released? We can't. If that code ever made any sense than in ancient pre-history in IRIX. > If an open file's link count is incorrectly low, it can't get freed > until after all the open file descriptors have been released, right? > Or is there some other vector for this? No. > I'm wondering if this ought to be: > > if (XFS_IS_CORRUPT(mp, !VFS_I(ip)->i_mode)) { > xfs_inode_mark_sick(ip); > return -EFSCORRUPTED; > } I wouldn't even bother with that.
On Mon, Jun 24, 2024 at 05:50:11PM +0200, Christoph Hellwig wrote: > On Mon, Jun 24, 2024 at 08:34:59AM -0700, Darrick J. Wong wrote: > > > - if (!S_ISREG(VFS_I(ip)->i_mode) || (VFS_I(ip)->i_mode == 0)) > > > > How would we encounter !i_mode regular files being released? > > We can't. If that code ever made any sense than in ancient pre-history > in IRIX. > > > If an open file's link count is incorrectly low, it can't get freed > > until after all the open file descriptors have been released, right? > > Or is there some other vector for this? > > No. > > > I'm wondering if this ought to be: > > > > if (XFS_IS_CORRUPT(mp, !VFS_I(ip)->i_mode)) { > > xfs_inode_mark_sick(ip); > > return -EFSCORRUPTED; > > } > > I wouldn't even bother with that. Oh, right, because xfs_iget checks that for us and bails out before the vfs can even get its hands on the file. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 38f946e3be2da3..9a9340aebe9d8a 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -1552,9 +1552,6 @@ xfs_release( xfs_mount_t *mp = ip->i_mount; int error = 0; - if (!S_ISREG(VFS_I(ip)->i_mode) || (VFS_I(ip)->i_mode == 0)) - return 0; - /* If this is a read-only mount, don't do this (would generate I/O) */ if (xfs_is_readonly(mp)) return 0;
xfs_release is only called from xfs_file_release, which is wired up as the f_op->release handler for regular files only. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_inode.c | 3 --- 1 file changed, 3 deletions(-)