open sets ext4_da_aops for DAX existing files

open sets ext4_da_aops for DAX existing files

[Kani, Toshi]

I noticed that both ext4_da_aops and ext4_dax_aops are used on DAX
mounted ext4 files.  Looking at 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

So, we set ext4_da_aops for existing files since S_DAX flag is set after
ext4_set_aops().

Thanks,
-Toshi

Re: open sets ext4_da_aops for DAX existing files

[Jeff Moyer]

"Kani, Toshi" <toshi.kani@hpe.com> writes:

> I noticed that both ext4_da_aops and ext4_dax_aops are used on DAX
> mounted ext4 files.  Looking at open() path:

Eek.  How did you notice this?

-Jeff

Re: open sets ext4_da_aops for DAX existing files

[Kani, Toshi]

On Mon, 2018-09-10 at 08:54 -0400, Jeff Moyer wrote:
> "Kani, Toshi" <toshi.kani@hpe.com> writes:
> 
> > I noticed that both ext4_da_aops and ext4_dax_aops are used on DAX
> > mounted ext4 files.  Looking at open() path:
> 
> Eek.  How did you notice this?

I tested sync path on DAX files to see if it flushes processor cache
properly. Then I noticed that it sometimes does, but sometimes doesn't.
Looking into function traces, I realized that ext4_dax_writepages() and
ext4_writepages() are called depending on their file status.

-Toshi

Re: open sets ext4_da_aops for DAX existing files

[Dan Williams]

On Mon, Sep 10, 2018 at 7:21 AM, Kani, Toshi <toshi.kani@hpe.com> wrote:
> On Mon, 2018-09-10 at 08:54 -0400, Jeff Moyer wrote:
>> "Kani, Toshi" <toshi.kani@hpe.com> writes:
>>
>> > I noticed that both ext4_da_aops and ext4_dax_aops are used on DAX
>> > mounted ext4 files.  Looking at open() path:
>>
>> Eek.  How did you notice this?
>
> I tested sync path on DAX files to see if it flushes processor cache
> properly. Then I noticed that it sometimes does, but sometimes doesn't.
> Looking into function traces, I realized that ext4_dax_writepages() and
> ext4_writepages() are called depending on their file status.

Yikes, and nice find!

Re: open sets ext4_da_aops for DAX existing files

[Jan Kara]

On Fri 07-09-18 21:23:19, Kani, Toshi wrote:
> I noticed that both ext4_da_aops and ext4_dax_aops are used on DAX
> mounted ext4 files.  Looking at 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
> 
> So, we set ext4_da_aops for existing files since S_DAX flag is set after
> ext4_set_aops().

Good catch. Will you send a fix? I.e., call ext4_set_inode_flags() earlier
in the ext4_iget()? Did this bug have any user visible manifestations?
Please also add:

Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086

so that stable automation picks this up. Thanks!

								Honza

Re: open sets ext4_da_aops for DAX existing files

[Kani, Toshi]

On Mon, 2018-09-10 at 16:29 +0200, Jan Kara wrote:
> On Fri 07-09-18 21:23:19, Kani, Toshi wrote:
> > I noticed that both ext4_da_aops and ext4_dax_aops are used on DAX
> > mounted ext4 files.  Looking at 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
> > 
> > So, we set ext4_da_aops for existing files since S_DAX flag is set after
> > ext4_set_aops().
> 
> Good catch. Will you send a fix? I.e., call ext4_set_inode_flags() earlier
> in the ext4_iget()? Did this bug have any user visible manifestations?

Yes, sync did not flush processor cache.

> Please also add:
> 
> Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
> 
> so that stable automation picks this up. Thanks!
> 

Will do.

Thanks,
-Toshi

RE: open sets ext4_da_aops for DAX existing files

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of Kani, Toshi
> Sent: Monday, September 10, 2018 9:52 AM
> To: jack@suse.cz
> Cc: linux-fsdevel@vger.kernel.org; linux-nvdimm@lists.01.org
> Subject: Re: open sets ext4_da_aops for DAX existing files
...
> > Good catch. Will you send a fix? I.e., call ext4_set_inode_flags() earlier
> > in the ext4_iget()? Did this bug have any user visible manifestations?
> 
> Yes, sync did not flush processor cache.
> 
> > Please also add:
> >
> > Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
> >
> > so that stable automation picks this up. Thanks!
> >
> 
> Will do.
> 

Also check if the same problem exists with ext2.

---
Robert Elliott, HPE Persistent Memory

Re: open sets ext4_da_aops for DAX existing files

[Jan Kara]

On Mon 10-09-18 17:58:10, Elliott, Robert (Persistent Memory) wrote:
> 
> 
> > -----Original Message-----
> > From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of Kani, Toshi
> > Sent: Monday, September 10, 2018 9:52 AM
> > To: jack@suse.cz
> > Cc: linux-fsdevel@vger.kernel.org; linux-nvdimm@lists.01.org
> > Subject: Re: open sets ext4_da_aops for DAX existing files
> ...
> > > Good catch. Will you send a fix? I.e., call ext4_set_inode_flags() earlier
> > > in the ext4_iget()? Did this bug have any user visible manifestations?
> > 
> > Yes, sync did not flush processor cache.
> > 
> > > Please also add:
> > >
> > > Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
> > >
> > > so that stable automation picks this up. Thanks!
> > >
> > 
> > Will do.
> > 
> 
> Also check if the same problem exists with ext2.

The problem also exists in ext2 so that needs fixing as well.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: open sets ext4_da_aops for DAX existing files

[Kani, Toshi]

On Tue, 2018-09-11 at 17:27 +0200, Jan Kara wrote:
> On Mon 10-09-18 17:58:10, Elliott, Robert (Persistent Memory) wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of Kani, Toshi
> > > Sent: Monday, September 10, 2018 9:52 AM
> > > To: jack@suse.cz
> > > Cc: linux-fsdevel@vger.kernel.org; linux-nvdimm@lists.01.org
> > > Subject: Re: open sets ext4_da_aops for DAX existing files
> > 
> > ...
> > > > Good catch. Will you send a fix? I.e., call ext4_set_inode_flags() earlier
> > > > in the ext4_iget()? Did this bug have any user visible manifestations?
> > > 
> > > Yes, sync did not flush processor cache.
> > > 
> > > > Please also add:
> > > > 
> > > > Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086
> > > > 
> > > > so that stable automation picks this up. Thanks!
> > > > 
> > > 
> > > Will do.
> > > 
> > 
> > Also check if the same problem exists with ext2.
> 
> The problem also exists in ext2 so that needs fixing as well.

Right.  Once my ext4 patch 2/2 is reviewed, I will duplicate it for
ext2.

Thanks,
-Toshi