Message ID | 20190115170849.GC12689@magnolia (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xfs: use generic fillattr to reduce redundant code | expand |
On Tue, Jan 15, 2019 at 09:08:49AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Refactor xfs_vn_getattr to use generic_fillattr to fill out parts of the > kstat structure instead of open-coding the same pieces. This eliminates > redundant code and fixes a bug where we fail to set the AUTOMOUNT > attribute. Obviously, we retain all the xfs-specific parts. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_iops.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index f48ffd7a8d3e..169bd7824479 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -501,16 +501,9 @@ xfs_vn_getattr( > if (XFS_FORCED_SHUTDOWN(mp)) > return -EIO; > > + generic_fillattr(inode, stat); > stat->size = XFS_ISIZE(ip); Maybe a comment to indicate that we're overwriting some of the fields that generic_fillattr() also set from the VFS inode, so we need to call it first before filling out the specific XFS information? Cheers, Dave.
On Tue, Jan 15, 2019 at 09:08:49AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Refactor xfs_vn_getattr to use generic_fillattr to fill out parts of the > kstat structure instead of open-coding the same pieces. This eliminates > redundant code and fixes a bug where we fail to set the AUTOMOUNT > attribute. Obviously, we retain all the xfs-specific parts. I find the idea of writing the same values twice for something like stat that is pretty performane critical rather odd. I'd much rather just fix the XFS version up to set the automount attribute, and clear STATX_ATIME for noatime inodes.
On Tue, Jan 15, 2019 at 01:13:06PM -0800, Christoph Hellwig wrote: > On Tue, Jan 15, 2019 at 09:08:49AM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Refactor xfs_vn_getattr to use generic_fillattr to fill out parts of the > > kstat structure instead of open-coding the same pieces. This eliminates > > redundant code and fixes a bug where we fail to set the AUTOMOUNT > > attribute. Obviously, we retain all the xfs-specific parts. > > I find the idea of writing the same values twice for something like > stat that is pretty performane critical rather odd. > > I'd much rather just fix the XFS version up to set the automount > attribute, and clear STATX_ATIME for noatime inodes. Actually, the proper fix is to move these checks to what actually is common code, something like this: -- From 3f82b9fce150a8af6b98bae686bbd24cbe3388ab Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Tue, 15 Jan 2019 22:31:50 +0100 Subject: fs: move generic stat response attr handling to vfs_getattr_nosec generic_fillattr is an optional helper that isn't used by all filesystems, move handling purely based on inode flags to vfs_getattr_nosec, which is common code. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/stat.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/fs/stat.c b/fs/stat.c index adbfcd86c81b..9600ff1ea8df 100644 --- a/fs/stat.c +++ b/fs/stat.c @@ -45,11 +45,6 @@ void generic_fillattr(struct inode *inode, struct kstat *stat) stat->ctime = inode->i_ctime; stat->blksize = i_blocksize(inode); stat->blocks = inode->i_blocks; - - if (IS_NOATIME(inode)) - stat->result_mask &= ~STATX_ATIME; - if (IS_AUTOMOUNT(inode)) - stat->attributes |= STATX_ATTR_AUTOMOUNT; } EXPORT_SYMBOL(generic_fillattr); @@ -75,11 +70,19 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat, stat->result_mask |= STATX_BASIC_STATS; request_mask &= STATX_ALL; query_flags &= KSTAT_QUERY_FLAGS; - if (inode->i_op->getattr) - return inode->i_op->getattr(path, stat, request_mask, - query_flags); + if (inode->i_op->getattr) { + int ret = inode->i_op->getattr(path, stat, request_mask, + query_flags); + if (ret) + return ret; + } else { + generic_fillattr(inode, stat); + } - generic_fillattr(inode, stat); + if (IS_NOATIME(inode)) + stat->result_mask &= ~STATX_ATIME; + if (IS_AUTOMOUNT(inode)) + stat->attributes |= STATX_ATTR_AUTOMOUNT; return 0; } EXPORT_SYMBOL(vfs_getattr_nosec);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index f48ffd7a8d3e..169bd7824479 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -501,16 +501,9 @@ xfs_vn_getattr( if (XFS_FORCED_SHUTDOWN(mp)) return -EIO; + generic_fillattr(inode, stat); stat->size = XFS_ISIZE(ip); - stat->dev = inode->i_sb->s_dev; - stat->mode = inode->i_mode; - stat->nlink = inode->i_nlink; - stat->uid = inode->i_uid; - stat->gid = inode->i_gid; stat->ino = ip->i_ino; - stat->atime = inode->i_atime; - stat->mtime = inode->i_mtime; - stat->ctime = inode->i_ctime; stat->blocks = XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks); @@ -533,7 +526,6 @@ xfs_vn_getattr( case S_IFBLK: case S_IFCHR: stat->blksize = BLKDEV_IOSIZE; - stat->rdev = inode->i_rdev; break; default: if (XFS_IS_REALTIME_INODE(ip)) {