Hi Hongbo,
Thanks for the suggestion.
I checked the STATX_DIOALIGN specification while looking at the
implementation of other file systems, and I thought that if DIO
support is incomplete, the dio_xx_align member should be set to 0.
Due to the nature of NILFS2 as a log-structured file system, DIO
writes fall back to buffered io. (DIO reads are supported)
This is similar to the journal data mode of ext4 and the blkzoned
device support of f2fs, but in such case, these file systems return a
value of 0 (direct I/O not supported).
So, it's fine to respond to a STATX_DIOALIGN request, but I think the
value of dio_xx_align should be set to 0 to match these file systems.
In this sense, there may be no need to rush to support STATX_DIOALIGN
now. Do you still think it would be better to have it?
The following are some minor comments:
On Tue, Aug 27, 2024 at 10:58 AM Hongbo Li wrote:
>
> Add support for STATX_DIOALIGN to nilfs2, so that direct I/O alignment
> restrictions are exposed to userspace in a generic way.
>
> By default, nilfs2 uses the default getattr implemented at vfs layer,
> so we should implement getattr in nilfs2 to fill the dio_xx_align
> members. The nilfs2 does not have the special align requirements. So
> we use the default alignment setting from block layer.
> We have done the following test:
>
> [Before]
> ```
> ./statx_test /mnt/nilfs2/test
> statx(/mnt/nilfs2/test) = 0
> dio mem align:0
> dio offset align:0
> ```
>
> [After]
> ```
> ./statx_test /mnt/nilfs2/test
> statx(/mnt/nilfs2/test) = 0
> dio mem align:512
> dio offset align:512
> ```
>
> Signed-off-by: Hongbo Li <lihongbo22@xxxxxxxxxx>
> ---
> fs/nilfs2/file.c | 1 +
> fs/nilfs2/inode.c | 20 ++++++++++++++++++++
> fs/nilfs2/namei.c | 2 ++
> fs/nilfs2/nilfs.h | 2 ++
> 4 files changed, 25 insertions(+)
>
> diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
> index 0e3fc5ba33c7..5528918d4b96 100644
> --- a/fs/nilfs2/file.c
> +++ b/fs/nilfs2/file.c
> @@ -154,6 +154,7 @@ const struct file_operations nilfs_file_operations = {
>
> const struct inode_operations nilfs_file_inode_operations = {
> .setattr = nilfs_setattr,
> + .getattr = nilfs_getattr,
> .permission = nilfs_permission,
> .fiemap = nilfs_fiemap,
> .fileattr_get = nilfs_fileattr_get,
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 7340a01d80e1..b5bb2c2de32c 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -1001,6 +1001,26 @@ int nilfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> return err;
> }
>
> +int nilfs_getattr(struct mnt_idmap *idmap, const struct path *path,
> + struct kstat *stat, u32 request_mask, unsigned int query_flags)
> +{
> + struct inode *const inode = d_inode(path->dentry);
> + struct block_device *bdev = inode->i_sb->s_bdev;
> + unsigned int blksize = (1 << inode->i_blkbits);
> +
> + if ((request_mask & STATX_DIOALIGN) && S_ISREG(inode->i_mode)) {
> + stat->result_mask |= STATX_DIOALIGN;
> +
> + if (bdev)
> + blksize = bdev_logical_block_size(bdev);
I don't think there's any need to check that bdev is NULL, but is
there a reason?
If sb->s_bdev can be NULL, I think that for such devices, a NULL
pointer dereference bug will occur in the mount path.
That's why I was concerned about this.
> + stat->dio_mem_align = blksize;
> + stat->dio_offset_align = blksize;
> + }
> +
> + generic_fillattr(idmap, request_mask, inode, stat);
> + return 0;
> +}
> +
> int nilfs_permission(struct mnt_idmap *idmap, struct inode *inode,
> int mask)
> {
> diff --git a/fs/nilfs2/namei.c b/fs/nilfs2/namei.c
> index c950139db6ef..ad56f4f8be1f 100644
> --- a/fs/nilfs2/namei.c
> +++ b/fs/nilfs2/namei.c
> @@ -546,6 +546,7 @@ const struct inode_operations nilfs_dir_inode_operations = {
> .mknod = nilfs_mknod,
> .rename = nilfs_rename,
> .setattr = nilfs_setattr,
> + .getattr = nilfs_getattr,
In the case of directories, the STATX_DIOALIGN request is ignored, so
I don't think this is necessary for now. (It can be added in the
future when supporting other optional getattr requests/responses).
> .permission = nilfs_permission,
> .fiemap = nilfs_fiemap,
> .fileattr_get = nilfs_fileattr_get,
> @@ -554,6 +555,7 @@ const struct inode_operations nilfs_dir_inode_operations = {
>
> const struct inode_operations nilfs_special_inode_operations = {
> .setattr = nilfs_setattr,
> + .getattr = nilfs_getattr,
> .permission = nilfs_permission,
> };
Ditto.
>
> diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
> index 4017f7856440..98a8b28ca1db 100644
> --- a/fs/nilfs2/nilfs.h
> +++ b/fs/nilfs2/nilfs.h
> @@ -280,6 +280,8 @@ extern void nilfs_truncate(struct inode *);
> extern void nilfs_evict_inode(struct inode *);
> extern int nilfs_setattr(struct mnt_idmap *, struct dentry *,
> struct iattr *);
> +extern int nilfs_getattr(struct mnt_idmap *idmap, const struct path *path,
> + struct kstat *stat, u32 request_mask, unsigned int query_flags);
Do not add the "extern" directive to new function declarations.
We are moving towards eliminating the extern declarator from function
declarations whenever possible.
> extern void nilfs_write_failed(struct address_space *mapping, loff_t to);
> int nilfs_permission(struct mnt_idmap *idmap, struct inode *inode,
> int mask);
> --
> 2.34.1
>
That's all for my comments.
Thanks,
Ryusuke Konishi
