On Apr 22, 2025 / 16:15, hch wrote:
> Turns out this doesn't work. We used to have the request_mask, but it
> got removed in 25fbcd62d2e1 ("bdev: use bdev_io_min() for statx block
> size") so that stat can expose the block device min I/O size in
> st_blkdev, and as the blksize doesn't have it's own request_mask flag
> is hard to special case.
>
> So maybe the better question is why devtmpfs even calls into
> vfs_getattr? As far as I can tell handle_remove is only ever called on
> the actual devtmpfs file system, so we don't need to go through the
> VFS to query i_mode. i.e. the patch should also fix the issue. The
> modify_change is probably not needed either, but for now I'm aiming
> for the minimal fix.
>
> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> index 6dd1a8860f1c..53fb0829eb7b 100644
> --- a/drivers/base/devtmpfs.c
> +++ b/drivers/base/devtmpfs.c
> @@ -296,7 +296,7 @@ static int delete_path(const char *nodepath)
> return err;
> }
>
> -static int dev_mynode(struct device *dev, struct inode *inode, struct kstat *stat)
> +static int dev_mynode(struct device *dev, struct inode *inode)
> {
> /* did we create it */
> if (inode->i_private != &thread)
> @@ -304,13 +304,13 @@ static int dev_mynode(struct device *dev, struct inode *inode, struct kstat *sta
>
> /* does the dev_t match */
> if (is_blockdev(dev)) {
> - if (!S_ISBLK(stat->mode))
> + if (!S_ISBLK(inode->i_mode))
> return 0;
> } else {
> - if (!S_ISCHR(stat->mode))
> + if (!S_ISCHR(inode->i_mode))
> return 0;
> }
> - if (stat->rdev != dev->devt)
> + if (inode->i_rdev != dev->devt)
> return 0;
>
> /* ours */
> @@ -321,8 +321,7 @@ static int handle_remove(const char *nodename, struct device *dev)
> {
> struct path parent;
> struct dentry *dentry;
> - struct kstat stat;
> - struct path p;
> + struct inode *inode;
> int deleted = 0;
> int err;
>
> @@ -330,11 +329,8 @@ static int handle_remove(const char *nodename, struct device *dev)
> if (IS_ERR(dentry))
> return PTR_ERR(dentry);
>
> - p.mnt = parent.mnt;
> - p.dentry = dentry;
> - err = vfs_getattr(&p, &stat, STATX_TYPE | STATX_MODE,
> - AT_STATX_SYNC_AS_STAT);
> - if (!err && dev_mynode(dev, d_inode(dentry), &stat)) {
> + inode = d_inode(dentry);
> + if (dev_mynode(dev, inode)) {
> struct iattr newattrs;
> /*
> * before unlinking this node, reset permissions
> @@ -342,7 +338,7 @@ static int handle_remove(const char *nodename, struct device *dev)
> */
> newattrs.ia_uid = GLOBAL_ROOT_UID;
> newattrs.ia_gid = GLOBAL_ROOT_GID;
> - newattrs.ia_mode = stat.mode & ~0777;
> + newattrs.ia_mode = inode->i_mode & ~0777;
> newattrs.ia_valid =
> ATTR_UID|ATTR_GID|ATTR_MODE;
> inode_lock(d_inode(dentry));
I evaluated this new fix also, and confirmed it avoids the hang. Good.
I also ran whole blktests and observed no other regression. Thanks.
Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
