nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Toshi Kani <toshi.kani@hpe.com>
Cc: tytso@mit.edu, linux-nvdimm@lists.01.org,
	linux-kernel@vger.kernel.org, adilger.kernel@dilger.ca,
	jack@suse.cz, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 1/2] ext4, dax: update dax check to skip journal inode
Date: Wed, 12 Sep 2018 11:24:22 +0200	[thread overview]
Message-ID: <20180912092422.GA7782@quack2.suse.cz> (raw)
In-Reply-To: <20180911154246.6844-2-toshi.kani@hpe.com>

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 <toshi.kani@hpe.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>

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 <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

  parent reply	other threads:[~2018-09-12  9:24 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-11 15:42 [PATCH 0/2] fix sync to flush processor cache for ext4 DAX files Toshi Kani
2018-09-11 15:42 ` [PATCH 1/2] ext4, dax: update dax check to skip journal inode Toshi Kani
2018-09-11 17:59   ` Dan Williams
2018-09-11 18:11     ` Kani, Toshi
2018-09-12  9:24   ` Jan Kara [this message]
2018-09-12 15:47     ` Kani, Toshi
2018-09-12 16:20       ` Jan Kara
2018-09-12 16:52         ` Kani, Toshi
2018-09-11 15:42 ` [PATCH 2/2] ext4, dax: set ext4_dax_aops for dax files Toshi Kani
2018-09-11 18:15   ` Dan Williams
2018-09-11 18:41     ` Kani, Toshi
2018-09-12  9:31     ` Jan Kara
2018-09-12 16:08       ` Kani, Toshi
2018-09-12 16:41         ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180912092422.GA7782@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=adilger.kernel@dilger.ca \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=toshi.kani@hpe.com \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).