Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E9E79C4360F for ; Thu, 4 Apr 2019 09:31:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B8D5E205F4 for ; Thu, 4 Apr 2019 09:31:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732787AbfDDJJQ (ORCPT ); Thu, 4 Apr 2019 05:09:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59218 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732776AbfDDJJM (ORCPT ); Thu, 4 Apr 2019 05:09:12 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 65D43C057E09; Thu, 4 Apr 2019 09:09:11 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3D1065DD97; Thu, 4 Apr 2019 09:09:11 +0000 (UTC) Received: from zmail21.collab.prod.int.phx2.redhat.com (zmail21.collab.prod.int.phx2.redhat.com [10.5.83.24]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id 8748941F3C; Thu, 4 Apr 2019 09:09:10 +0000 (UTC) Date: Thu, 4 Apr 2019 05:09:10 -0400 (EDT) From: Pankaj Gupta To: "Darrick J. Wong" , Dave Chinner Cc: linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-acpi@vger.kernel.org, qemu-devel@nongnu.org, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org, dan j williams , zwisler@kernel.org, vishal l verma , dave jiang , mst@redhat.com, jasowang@redhat.com, willy@infradead.org, rjw@rjwysocki.net, hch@infradead.org, lenb@kernel.org, jack@suse.cz, tytso@mit.edu, adilger kernel , lcapitulino@redhat.com, kwolf@redhat.com, imammedo@redhat.com, jmoyer@redhat.com, nilal@redhat.com, riel@surriel.com, stefanha@redhat.com, aarcange@redhat.com, david@redhat.com, cohuck@redhat.com, xiaoguangrong eric Message-ID: <651986267.17402634.1554368950178.JavaMail.zimbra@redhat.com> In-Reply-To: <1508883801.17367965.1554358418550.JavaMail.zimbra@redhat.com> References: <20190403104018.23947-1-pagupta@redhat.com> <20190403104018.23947-6-pagupta@redhat.com> <20190403220912.GB26298@dastard> <20190403223921.GM5147@magnolia> <1508883801.17367965.1554358418550.JavaMail.zimbra@redhat.com> Subject: Re: [PATCH v4 5/5] xfs: disable map_sync for async flush MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.65.161.59, 10.4.195.10] Thread-Topic: disable map_sync for async flush Thread-Index: 3lcRs/qAvRTCJfnvvKOwPX3OPYStPt5rTV2c X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 04 Apr 2019 09:09:12 +0000 (UTC) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org > > On Thu, Apr 04, 2019 at 09:09:12AM +1100, Dave Chinner wrote: > > > On Wed, Apr 03, 2019 at 04:10:18PM +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 > > > > --- > > > > 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 1f2e2845eb76..dced2eb8c91a 100644 > > > > --- a/fs/xfs/xfs_file.c > > > > +++ b/fs/xfs/xfs_file.c > > > > @@ -1203,6 +1203,14 @@ xfs_file_mmap( > > > > if (!IS_DAX(file_inode(filp)) && (vma->vm_flags & VM_SYNC)) > > > > return -EOPNOTSUPP; > > > > > > > > + /* We don't support synchronous mappings with DAX files if > > > > + * dax_device is not synchronous. > > > > + */ > > > > + if (IS_DAX(file_inode(filp)) && !dax_synchronous( > > > > + 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))) > > > > > > All this ad hoc IS_DAX conditional logic is getting pretty nasty. > > > > > > xfs_file_mmap( > > > .... > > > { > > > struct inode *inode = file_inode(filp); > > > > > > if (vma->vm_flags & VM_SYNC) { > > > if (!IS_DAX(inode)) > > > return -EOPNOTSUPP; > > > if (!dax_synchronous(xfs_find_daxdev_for_inode(inode)) > > > return -EOPNOTSUPP; > > > } > > > > > > file_accessed(filp); > > > vma->vm_ops = &xfs_file_vm_ops; > > > if (IS_DAX(inode)) > > > vma->vm_flags |= VM_HUGEPAGE; > > > return 0; > > > } > > > > > > > > > Even better, factor out all the "MAP_SYNC supported" checks into a > > > helper so that the filesystem code just doesn't have to care about > > > the details of checking for DAX+MAP_SYNC support.... > > > > Seconded, since ext4 has nearly the same flag validation logic. > Only issue with this I see is we need the helper function only for supported filesystems ext4 & xfs (right now). If I create the function in "fs.h" it will be compiled for every filesystem, even for those don't need it. Sample patch below, does below patch is near to what you have in mind? ================= diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 1f2e2845eb76..614995170cac 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1196,12 +1196,17 @@ xfs_file_mmap( struct file *filp, struct vm_area_struct *vma) { + struct dax_device *dax_dev = xfs_find_daxdev_for_inode(file_inode(filp)); + /* - * We don't support synchronous mappings for non-DAX files. At least - * until someone comes with a sensible use case. + * We don't support synchronous mappings for non-DAX files and + * for DAX files if underneath dax_device is not synchronous. */ - if (!IS_DAX(file_inode(filp)) && (vma->vm_flags & VM_SYNC)) - return -EOPNOTSUPP; + if (vma->vm_flags & VM_SYNC) { + int err = is_synchronous(filp, dax_dev); + if (err) + return err; + } file_accessed(filp); vma->vm_ops = &xfs_file_vm_ops; diff --git a/include/linux/fs.h b/include/linux/fs.h index 8b42df09b04c..add017de3dd7 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2162,6 +2162,20 @@ static inline void file_accessed(struct file *file) touch_atime(&file->f_path); } +struct dax_device; +extern bool dax_synchronous(struct dax_device *dax_dev); +static inline int is_synchronous(struct file *filp, struct dax_device *dax_dev) +{ + struct inode *inode = file_inode(filp); + + if (!IS_DAX(inode)) + return -EOPNOTSUPP; + if (!dax_synchronous(dax_dev)) + return -EOPNOTSUPP; + + return 0; +} + int sync_inode(struct inode *inode, struct writeback_control *wbc); int sync_inode_metadata(struct inode *inode, int wait); --------- Thanks, Pankaj