Ext4 mount path calls ext4_iget() to obtain the journal inode. This
inode does not support DAX, and 'ext4_da_aops' needs to be set. It
currently works for the DAX mount case because ext4_iget() always set
'ext4_da_aops' to any regular files.
ext4_fill_super
ext4_load_journal
ext4_get_journal_inode
ext4_iget
In preparation to fix ext4_iget() to set 'ext4_dax_aops' for DAX files,
update ext4_should_use_dax() to return false for the journal inode.
Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
Signed-off-by: Toshi Kani <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: "Theodore Ts'o" <[email protected]>
Cc: Andreas Dilger <[email protected]>
---
fs/ext4/ext4_jbd2.h | 8 ++++++++
fs/ext4/inode.c | 2 ++
2 files changed, 10 insertions(+)
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 15b6dd733780..9847dc980a0d 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -437,6 +437,14 @@ static inline int ext4_should_writeback_data(struct inode *inode)
return ext4_inode_journal_mode(inode) & EXT4_INODE_WRITEBACK_DATA_MODE;
}
+static inline int ext4_is_journal_inode(struct inode *inode)
+{
+ struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+ unsigned int journal_inum = le32_to_cpu(sbi->s_es->s_journal_inum);
+
+ return journal_inum && (journal_inum == inode->i_ino);
+}
+
/*
* This function controls whether or not we should try to go down the
* dioread_nolock code paths, which makes it safe to avoid taking
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index d0dd585add6a..775cd9b4af55 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4686,6 +4686,8 @@ static bool ext4_should_use_dax(struct inode *inode)
return false;
if (ext4_should_journal_data(inode))
return false;
+ if (ext4_is_journal_inode(inode))
+ return false;
if (ext4_has_inline_data(inode))
return false;
if (ext4_encrypted_inode(inode))
On Tue, Sep 11, 2018 at 8:42 AM, Toshi Kani <[email protected]> wrote:
> Ext4 mount path calls ext4_iget() to obtain the journal inode. This
> inode does not support DAX, and 'ext4_da_aops' needs to be set. It
> currently works for the DAX mount case because ext4_iget() always set
> 'ext4_da_aops' to any regular files.
>
> ext4_fill_super
> ext4_load_journal
> ext4_get_journal_inode
> ext4_iget
>
> In preparation to fix ext4_iget() to set 'ext4_dax_aops' for DAX files,
> update ext4_should_use_dax() to return false for the journal inode.
>
> Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
Just a minor format recommendation:
Fixes: 5f0663bb4a64 ("ext4, dax: introduce ext4_dax_aops")
Cc: <[email protected]>
> Signed-off-by: Toshi Kani <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: "Theodore Ts'o" <[email protected]>
> Cc: Andreas Dilger <[email protected]>
> ---
> fs/ext4/ext4_jbd2.h | 8 ++++++++
> fs/ext4/inode.c | 2 ++
> 2 files changed, 10 insertions(+)
>
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index 15b6dd733780..9847dc980a0d 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -437,6 +437,14 @@ static inline int ext4_should_writeback_data(struct inode *inode)
> return ext4_inode_journal_mode(inode) & EXT4_INODE_WRITEBACK_DATA_MODE;
> }
>
> +static inline int ext4_is_journal_inode(struct inode *inode)
> +{
> + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> + unsigned int journal_inum = le32_to_cpu(sbi->s_es->s_journal_inum);
> +
> + return journal_inum && (journal_inum == inode->i_ino);
> +}
> +
> /*
> * This function controls whether or not we should try to go down the
> * dioread_nolock code paths, which makes it safe to avoid taking
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d0dd585add6a..775cd9b4af55 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4686,6 +4686,8 @@ static bool ext4_should_use_dax(struct inode *inode)
> return false;
> if (ext4_should_journal_data(inode))
> return false;
> + if (ext4_is_journal_inode(inode))
> + return false;
This looks good to me.
Reviewed-by: Dan Williams <[email protected]>
On Tue, 2018-09-11 at 10:59 -0700, Dan Williams wrote:
> On Tue, Sep 11, 2018 at 8:42 AM, Toshi Kani <[email protected]> wrote:
> > Ext4 mount path calls ext4_iget() to obtain the journal inode. This
> > inode does not support DAX, and 'ext4_da_aops' needs to be set. It
> > currently works for the DAX mount case because ext4_iget() always set
> > 'ext4_da_aops' to any regular files.
> >
> > ext4_fill_super
> > ext4_load_journal
> > ext4_get_journal_inode
> > ext4_iget
> >
> > In preparation to fix ext4_iget() to set 'ext4_dax_aops' for DAX files,
> > update ext4_should_use_dax() to return false for the journal inode.
> >
> > Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
>
> Just a minor format recommendation:
>
> Fixes: 5f0663bb4a64 ("ext4, dax: introduce ext4_dax_aops")
> Cc: <[email protected]>
Will do.
>
> This looks good to me.
>
> Reviewed-by: Dan Williams <[email protected]>
Thanks!
-Toshi
On Tue 11-09-18 09:42:45, Toshi Kani wrote:
> Ext4 mount path calls ext4_iget() to obtain the journal inode. This
> inode does not support DAX, and 'ext4_da_aops' needs to be set. It
> currently works for the DAX mount case because ext4_iget() always set
> 'ext4_da_aops' to any regular files.
>
> ext4_fill_super
> ext4_load_journal
> ext4_get_journal_inode
> ext4_iget
>
> In preparation to fix ext4_iget() to set 'ext4_dax_aops' for DAX files,
> update ext4_should_use_dax() to return false for the journal inode.
>
> Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
> Signed-off-by: Toshi Kani <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: "Theodore Ts'o" <[email protected]>
> Cc: Andreas Dilger <[email protected]>
The journal inode is similar to any other 'system' inode we have in ext4.
We don't really expect any of the address space operations to be called for
it because we don't use page cache with these inodes. Very similar
situation is with e.g. quota files. So to me it seems this patch is not
really necessary. Or did you observe any bad effects without this patch?
That being said I agree that it would be a good cleanup to define something
like ext4_is_system_inode() and disable DAX for these inodes just because
they are special. But I don't see a need for this as a part of your fix.
Honza
> ---
> fs/ext4/ext4_jbd2.h | 8 ++++++++
> fs/ext4/inode.c | 2 ++
> 2 files changed, 10 insertions(+)
>
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index 15b6dd733780..9847dc980a0d 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -437,6 +437,14 @@ static inline int ext4_should_writeback_data(struct inode *inode)
> return ext4_inode_journal_mode(inode) & EXT4_INODE_WRITEBACK_DATA_MODE;
> }
>
> +static inline int ext4_is_journal_inode(struct inode *inode)
> +{
> + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> + unsigned int journal_inum = le32_to_cpu(sbi->s_es->s_journal_inum);
> +
> + return journal_inum && (journal_inum == inode->i_ino);
> +}
> +
> /*
> * This function controls whether or not we should try to go down the
> * dioread_nolock code paths, which makes it safe to avoid taking
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d0dd585add6a..775cd9b4af55 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4686,6 +4686,8 @@ static bool ext4_should_use_dax(struct inode *inode)
> return false;
> if (ext4_should_journal_data(inode))
> return false;
> + if (ext4_is_journal_inode(inode))
> + return false;
> if (ext4_has_inline_data(inode))
> return false;
> if (ext4_encrypted_inode(inode))
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, 2018-09-12 at 11:24 +0200, Jan Kara wrote:
> On Tue 11-09-18 09:42:45, Toshi Kani wrote:
> > Ext4 mount path calls ext4_iget() to obtain the journal inode. This
> > inode does not support DAX, and 'ext4_da_aops' needs to be set. It
> > currently works for the DAX mount case because ext4_iget() always set
> > 'ext4_da_aops' to any regular files.
> >
> > ext4_fill_super
> > ext4_load_journal
> > ext4_get_journal_inode
> > ext4_iget
> >
> > In preparation to fix ext4_iget() to set 'ext4_dax_aops' for DAX files,
> > update ext4_should_use_dax() to return false for the journal inode.
> >
> > Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
> > Signed-off-by: Toshi Kani <[email protected]>
> > Cc: Jan Kara <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Cc: "Theodore Ts'o" <[email protected]>
> > Cc: Andreas Dilger <[email protected]>
>
> The journal inode is similar to any other 'system' inode we have in ext4.
> We don't really expect any of the address space operations to be called for
> it because we don't use page cache with these inodes. Very similar
> situation is with e.g. quota files. So to me it seems this patch is not
> really necessary. Or did you observe any bad effects without this patch?
Yes, without this change, mount fails with the error below. I believe
.bmap operation in ext4_da_aops is necessary for this case.
jbd2_journal_init_inode: Cannot locate journal superblock
EXT4-fs
(pmem1): Could not load journal inode
> That being said I agree that it would be a good cleanup to define something
> like ext4_is_system_inode() and disable DAX for these inodes just because
> they are special. But I don't see a need for this as a part of your fix.
Is there other system inode we need to check for?
Thanks,
-Toshi
On Wed 12-09-18 15:47:12, Kani, Toshi wrote:
> On Wed, 2018-09-12 at 11:24 +0200, Jan Kara wrote:
> > On Tue 11-09-18 09:42:45, Toshi Kani wrote:
> > > Ext4 mount path calls ext4_iget() to obtain the journal inode. This
> > > inode does not support DAX, and 'ext4_da_aops' needs to be set. It
> > > currently works for the DAX mount case because ext4_iget() always set
> > > 'ext4_da_aops' to any regular files.
> > >
> > > ext4_fill_super
> > > ext4_load_journal
> > > ext4_get_journal_inode
> > > ext4_iget
> > >
> > > In preparation to fix ext4_iget() to set 'ext4_dax_aops' for DAX files,
> > > update ext4_should_use_dax() to return false for the journal inode.
> > >
> > > Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
> > > Signed-off-by: Toshi Kani <[email protected]>
> > > Cc: Jan Kara <[email protected]>
> > > Cc: Dan Williams <[email protected]>
> > > Cc: "Theodore Ts'o" <[email protected]>
> > > Cc: Andreas Dilger <[email protected]>
> >
> > The journal inode is similar to any other 'system' inode we have in ext4.
> > We don't really expect any of the address space operations to be called for
> > it because we don't use page cache with these inodes. Very similar
> > situation is with e.g. quota files. So to me it seems this patch is not
> > really necessary. Or did you observe any bad effects without this patch?
>
> Yes, without this change, mount fails with the error below. I believe
> .bmap operation in ext4_da_aops is necessary for this case.
>
> jbd2_journal_init_inode: Cannot locate journal superblock
> EXT4-fs
> (pmem1): Could not load journal inode
Ah, OK, forgot about this. Then how about adding:
.bmap = ext4_bmap,
to ext4_dax_aops? .bmap is completely fine for DAX inodes and there's no
reason to disallow it.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, 2018-09-12 at 18:20 +0200, Jan Kara wrote:
> On Wed 12-09-18 15:47:12, Kani, Toshi wrote:
> > On Wed, 2018-09-12 at 11:24 +0200, Jan Kara wrote:
> > > On Tue 11-09-18 09:42:45, Toshi Kani wrote:
> > > > Ext4 mount path calls ext4_iget() to obtain the journal inode. This
> > > > inode does not support DAX, and 'ext4_da_aops' needs to be set. It
> > > > currently works for the DAX mount case because ext4_iget() always set
> > > > 'ext4_da_aops' to any regular files.
> > > >
> > > > ext4_fill_super
> > > > ext4_load_journal
> > > > ext4_get_journal_inode
> > > > ext4_iget
> > > >
> > > > In preparation to fix ext4_iget() to set 'ext4_dax_aops' for DAX files,
> > > > update ext4_should_use_dax() to return false for the journal inode.
> > > >
> > > > Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
> > > > Signed-off-by: Toshi Kani <[email protected]>
> > > > Cc: Jan Kara <[email protected]>
> > > > Cc: Dan Williams <[email protected]>
> > > > Cc: "Theodore Ts'o" <[email protected]>
> > > > Cc: Andreas Dilger <[email protected]>
> > >
> > > The journal inode is similar to any other 'system' inode we have in ext4.
> > > We don't really expect any of the address space operations to be called for
> > > it because we don't use page cache with these inodes. Very similar
> > > situation is with e.g. quota files. So to me it seems this patch is not
> > > really necessary. Or did you observe any bad effects without this patch?
> >
> > Yes, without this change, mount fails with the error below. I believe
> > .bmap operation in ext4_da_aops is necessary for this case.
> >
> > jbd2_journal_init_inode: Cannot locate journal superblock
> > EXT4-fs
> > (pmem1): Could not load journal inode
>
> Ah, OK, forgot about this. Then how about adding:
> .bmap = ext4_bmap,
>
> to ext4_dax_aops? .bmap is completely fine for DAX inodes and there's no
> reason to disallow it.
Yes, mount -o dax succeeds with the added ext4_bmap in ext4_dax_aops.
Thanks!
-Toshi