From: Dan Williams <dan.j.williams@intel.com>
To: Toshi Kani <toshi.kani@hpe.com>
Cc: Theodore Ts'o <tytso@mit.edu>,
linux-nvdimm <linux-nvdimm@lists.01.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Andreas Dilger <adilger.kernel@dilger.ca>,
Jan Kara <jack@suse.cz>, linux-ext4 <linux-ext4@vger.kernel.org>
Subject: Re: [PATCH 2/2] ext4, dax: set ext4_dax_aops for dax files
Date: Tue, 11 Sep 2018 11:15:18 -0700 [thread overview]
Message-ID: <CAPcyv4iEZ7q0MV7n-_eovrJw_U9S3=pkY_nYwtw1jSBS7vPqCQ@mail.gmail.com> (raw)
In-Reply-To: <20180911154246.6844-3-toshi.kani@hpe.com>
On Tue, Sep 11, 2018 at 8:42 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> Sync syscall to an existing DAX file needs to flush processor cache,
> but it does not currently. This is because 'ext4_da_aops' is set to
> address_space_operations of existing DAX files, instead of 'ext4_dax_aops',
> since S_DAX flag is set after ext4_set_aops() in the open path.
>
> New file
> --------
> lookup_open
> ext4_create
> __ext4_new_inode
> ext4_set_inode_flags // Set S_DAX flag
> ext4_set_aops // Set aops to ext4_dax_aops
>
> Existing file
> -------------
> lookup_open
> ext4_lookup
> ext4_iget
> ext4_set_aops // Set aops to ext4_da_aops
> ext4_set_inode_flags // Set S_DAX flag
>
> Change ext4_iget() to call ext4_set_inode_flags() before ext4_set_aops().
>
> Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
Same format nit:
Fixes: 5f0663bb4a64 ("ext4, dax: introduce ext4_dax_aops")
Cc: <stable@vger.kernel.org>
> 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>
> ---
> fs/ext4/inode.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 775cd9b4af55..93cbbb859c40 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4998,6 +4998,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> if (ret)
> goto bad_inode;
>
> + ext4_set_inode_flags(inode);
> +
Hmm, does this have unintended behavior changes?
I notice that there are some checks for flags "IS_APPEND(inode) ||
IS_IMMUTABLE(inode)" *before* the call to ext4_set_inode_flags(). I
didn't look too much deeper at whether those checks are bogus, but it
would seem safer to do something like this for a lower risk fix.
Thoughts?
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index d0dd585add6a..1e9ab445c777 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4999,7 +4999,6 @@ struct inode *ext4_iget(struct super_block *sb,
unsigned long ino)
if (S_ISREG(inode->i_mode)) {
inode->i_op = &ext4_file_inode_operations;
inode->i_fop = &ext4_file_operations;
- ext4_set_aops(inode);
} else if (S_ISDIR(inode->i_mode)) {
inode->i_op = &ext4_dir_inode_operations;
inode->i_fop = &ext4_dir_operations;
@@ -5042,6 +5041,12 @@ struct inode *ext4_iget(struct super_block *sb,
unsigned long ino)
}
brelse(iloc.bh);
ext4_set_inode_flags(inode);
+ /*
+ * Now that we have determined whether DAX is enabled, set the
+ * proper address spaces operations
+ */
+ if (S_ISREG(inode->i_mode))
+ ext4_set_aops(inode);
unlock_new_inode(inode);
return inode;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
next prev parent reply other threads:[~2018-09-11 18:15 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
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 [this message]
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='CAPcyv4iEZ7q0MV7n-_eovrJw_U9S3=pkY_nYwtw1jSBS7vPqCQ@mail.gmail.com' \
--to=dan.j.williams@intel.com \
--cc=adilger.kernel@dilger.ca \
--cc=jack@suse.cz \
--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).