2019-01-09 13:56:22

by Pankaj Gupta

[permalink] [raw]
Subject: [PATCH v3 4/5] ext4: disable map_sync for virtio pmem

Virtio pmem provides asynchronous host page cache flush
mechanism. We don't support 'MAP_SYNC' with virtio pmem
and ext4.

Signed-off-by: Pankaj Gupta <[email protected]>
---
fs/ext4/file.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 69d65d4..e54f48b 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -360,8 +360,10 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
{
struct inode *inode = file->f_mapping->host;
+ struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+ struct dax_device *dax_dev = sbi->s_daxdev;

- if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+ if (unlikely(ext4_forced_shutdown(sbi)))
return -EIO;

/*
@@ -371,6 +373,13 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
if (!IS_DAX(file_inode(file)) && (vma->vm_flags & VM_SYNC))
return -EOPNOTSUPP;

+ /* We don't support synchronous mappings with guest direct access
+ * and virtio based host page cache flush mechanism.
+ */
+ if (IS_DAX(file_inode(file)) && virtio_pmem_host_cache_enabled(dax_dev)
+ && (vma->vm_flags & VM_SYNC))
+ return -EOPNOTSUPP;
+
file_accessed(file);
if (IS_DAX(file_inode(file))) {
vma->vm_ops = &ext4_dax_vm_ops;
--
2.9.3


2019-01-09 14:42:19

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH v3 4/5] ext4: disable map_sync for virtio pmem

On Wed 09-01-19 19:26:05, Pankaj Gupta wrote:
> Virtio pmem provides asynchronous host page cache flush
> mechanism. We don't support 'MAP_SYNC' with virtio pmem
> and ext4.
>
> Signed-off-by: Pankaj Gupta <[email protected]>
...
> @@ -371,6 +373,13 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
> if (!IS_DAX(file_inode(file)) && (vma->vm_flags & VM_SYNC))
> return -EOPNOTSUPP;
>
> + /* We don't support synchronous mappings with guest direct access
> + * and virtio based host page cache flush mechanism.
> + */
> + if (IS_DAX(file_inode(file)) && virtio_pmem_host_cache_enabled(dax_dev)
> + && (vma->vm_flags & VM_SYNC))
> + return -EOPNOTSUPP;
> +

Shouldn't there rather be some generic way of doing this? Having
virtio_pmem_host_cache_enabled() check in filesystem code just looks like
filesystem sniffing into details is should not care about... Maybe just
naming this (or having a wrapper) dax_dev_map_sync_supported()?

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2019-01-09 14:54:28

by Pankaj Gupta

[permalink] [raw]
Subject: Re: [PATCH v3 4/5] ext4: disable map_sync for virtio pmem


>
> On Wed 09-01-19 19:26:05, Pankaj Gupta wrote:
> > Virtio pmem provides asynchronous host page cache flush
> > mechanism. We don't support 'MAP_SYNC' with virtio pmem
> > and ext4.
> >
> > Signed-off-by: Pankaj Gupta <[email protected]>
> ...
> > @@ -371,6 +373,13 @@ static int ext4_file_mmap(struct file *file, struct
> > vm_area_struct *vma)
> > if (!IS_DAX(file_inode(file)) && (vma->vm_flags & VM_SYNC))
> > return -EOPNOTSUPP;
> >
> > + /* We don't support synchronous mappings with guest direct access
> > + * and virtio based host page cache flush mechanism.
> > + */
> > + if (IS_DAX(file_inode(file)) && virtio_pmem_host_cache_enabled(dax_dev)
> > + && (vma->vm_flags & VM_SYNC))
> > + return -EOPNOTSUPP;
> > +
>
> Shouldn't there rather be some generic way of doing this? Having
> virtio_pmem_host_cache_enabled() check in filesystem code just looks like
> filesystem sniffing into details is should not care about... Maybe just
> naming this (or having a wrapper) dax_dev_map_sync_supported()?

Thanks for the feedback.

Just wanted to avoid 'dax' in function name just to differentiate this with real dax.
But yes can add a wrapper: dax_dev_map_sync_supported()

Thanks,
Pankaj

2019-01-09 13:56:31

by Pankaj Gupta

[permalink] [raw]
Subject: [PATCH v3 5/5] xfs: disable map_sync for virtio pmem

Virtio pmem provides asynchronous host page cache flush
mechanism. we don't support 'MAP_SYNC' with virtio pmem
and xfs.

Signed-off-by: Pankaj Gupta <[email protected]>
---
fs/xfs/xfs_file.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index e474250..eae4aa4 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1190,6 +1190,14 @@ xfs_file_mmap(
if (!IS_DAX(file_inode(filp)) && (vma->vm_flags & VM_SYNC))
return -EOPNOTSUPP;

+ /* We don't support synchronous mappings with guest direct access
+ * and virtio based host page cache mechanism.
+ */
+ if (IS_DAX(file_inode(filp)) && virtio_pmem_host_cache_enabled(
+ xfs_find_daxdev_for_inode(file_inode(filp))) &&
+ (vma->vm_flags & VM_SYNC))
+ return -EOPNOTSUPP;
+
file_accessed(filp);
vma->vm_ops = &xfs_file_vm_ops;
if (IS_DAX(file_inode(filp)))
--
2.9.3

2019-01-09 19:15:30

by Darrick J. Wong

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] xfs: disable map_sync for virtio pmem

On Wed, Jan 09, 2019 at 08:17:36PM +0530, Pankaj Gupta wrote:
> Virtio pmem provides asynchronous host page cache flush
> mechanism. we don't support 'MAP_SYNC' with virtio pmem
> and xfs.
>
> Signed-off-by: Pankaj Gupta <[email protected]>
> ---
> fs/xfs/xfs_file.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index e474250..eae4aa4 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1190,6 +1190,14 @@ xfs_file_mmap(
> if (!IS_DAX(file_inode(filp)) && (vma->vm_flags & VM_SYNC))
> return -EOPNOTSUPP;
>
> + /* We don't support synchronous mappings with guest direct access
> + * and virtio based host page cache mechanism.
> + */
> + if (IS_DAX(file_inode(filp)) && virtio_pmem_host_cache_enabled(

Echoing what Jan said, this ought to be some sort of generic function
that tells us whether or not memory mapped from the dax device will
always still be accessible even after a crash (i.e. supports MAP_SYNC).

What if the underlying file on the host is itself on pmem and can be
MAP_SYNC'd? Shouldn't the guest be able to use MAP_SYNC as well?

--D

> + xfs_find_daxdev_for_inode(file_inode(filp))) &&
> + (vma->vm_flags & VM_SYNC))
> + return -EOPNOTSUPP;
> +
> file_accessed(filp);
> vma->vm_ops = &xfs_file_vm_ops;
> if (IS_DAX(file_inode(filp)))
> --
> 2.9.3
>

2019-01-09 18:08:26

by Pankaj Gupta

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] xfs: disable map_sync for virtio pmem


> > Virtio pmem provides asynchronous host page cache flush
> > mechanism. we don't support 'MAP_SYNC' with virtio pmem
> > and xfs.
> >
> > Signed-off-by: Pankaj Gupta <[email protected]>
> > ---
> > fs/xfs/xfs_file.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index e474250..eae4aa4 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1190,6 +1190,14 @@ xfs_file_mmap(
> > if (!IS_DAX(file_inode(filp)) && (vma->vm_flags & VM_SYNC))
> > return -EOPNOTSUPP;
> >
> > + /* We don't support synchronous mappings with guest direct access
> > + * and virtio based host page cache mechanism.
> > + */
> > + if (IS_DAX(file_inode(filp)) && virtio_pmem_host_cache_enabled(
>
> Echoing what Jan said, this ought to be some sort of generic function
> that tells us whether or not memory mapped from the dax device will
> always still be accessible even after a crash (i.e. supports MAP_SYNC).

o.k
>
> What if the underlying file on the host is itself on pmem and can be
> MAP_SYNC'd? Shouldn't the guest be able to use MAP_SYNC as well?

Guest MAP_SYNC on actual host pmem will sync guest metadata, as guest
writes are persistent on actual pmem. Host side Qemu MAP_SYNC enabling
work for pmem is in progress. It will make sure metadata at host side
is in consistent state after a crash or any other metadata corruption
operation.

For virtio-pmem, we are emulating pmem device over regular storage on
host side. Guest need to call fsync followed by write to make sure
guest metadata is in consistent state(or journalled). Host backing file
data & metadata will also be persistent after guest to host fsync.

Thanks,
Pankaj