Message ID | e988d4c4-f8b2-4cdb-9915-790abf69bfed@kernel.dk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [GIT,PULL] Block updates for 6.11-rc1 | expand |
On Thu, 11 Jul 2024 at 22:44, Jens Axboe <axboe@kernel.dk> wrote: > > Note the statx change for atomic writes will cause a trivial conflict > with a patch that went into the 6.9 kernel via the vfs tree later than > what the 6.11 block tree was based on. Easy to resolve, only mentioning > it for completeness sake. It may be easy to resolve, but as I was resolving it I threw up in my mouth a little. That whole struct inode *backing_inode; ... backing_inode = d_backing_inode(path.dentry); if (S_ISBLK(backing_inode->i_mode)) bdev_statx(backing_inode, stat, request_mask); does not belong in the generic stat helper. The whole *point* of bdev_statx() is to not need that kind of bdev-specific knowledge. So I rewrote it to be if (S_ISBLK(stat->mode)) bdev_statx(path, stat, request_mask); instead, and that whole "backing_inode" stuff is now inside bdev_statx(). Because wasn't that the whole point of abstracting this out, so that block devices could do their own thing? And if the backing inode is a special block device, but the "front" inode doesn't say S_IFBLK, something is very screwed up. But hey, maybe I screwed something up. But randomly having that whole "d_backing_inode()" logic in there seemed entirely against the abstraction layering. Linus
The pull request you sent on Thu, 11 Jul 2024 23:44:25 -0600:
> git://git.kernel.dk/linux.git tags/for-6.11/block-20240710
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/3e7819886281e077e82006fe4804b0d6b0f5643b
Thank you!
On 15/07/2024 22:32, Linus Torvalds wrote: >> Note the statx change for atomic writes will cause a trivial conflict >> with a patch that went into the 6.9 kernel via the vfs tree later than >> what the 6.11 block tree was based on. Easy to resolve, only mentioning >> it for completeness sake. > It may be easy to resolve, but as I was resolving it I threw up in my > mouth a little. > > That whole > > struct inode *backing_inode; > ... > backing_inode = d_backing_inode(path.dentry); > if (S_ISBLK(backing_inode->i_mode)) > bdev_statx(backing_inode, stat, request_mask); > > does not belong in the generic stat helper. The whole*point* of > bdev_statx() is to not need that kind of bdev-specific knowledge. > > So I rewrote it to be > > if (S_ISBLK(stat->mode)) > bdev_statx(path, stat, request_mask); > > instead, and that whole "backing_inode" stuff is now inside bdev_statx(). > > Because wasn't that the whole point of abstracting this out, so that > block devices could do their own thing? > > And if the backing inode is a special block device, but the "front" > inode doesn't say S_IFBLK, something is very screwed up. > > But hey, maybe I screwed something up. But randomly having that whole > "d_backing_inode()" logic in there seemed entirely against the > abstraction layering. This code bdev statx originates in 2d985f8c6b91b. In looking at the lore history for that commit, Eric was passing the path at one stage: https://lore.kernel.org/linux-fsdevel/YrgPOHarxLdMt2m2@infradead.org/ With that change (to pass the backing inode) came that big comment in bdev_statx{_dioalign}() about what backing_inode is - I am not sure on the value of that now. Anyway, the change works ok for me. John