From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH v5 09/17] dax: coordinate locking for offsets in PMD range Date: Tue, 11 Oct 2016 09:04:09 +0200 Message-ID: <20161011070409.GC6952@quack2.suse.cz> References: <1475874544-24842-1-git-send-email-ross.zwisler@linux.intel.com> <1475874544-24842-10-git-send-email-ross.zwisler@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-kernel@vger.kernel.org, Theodore Ts'o , Alexander Viro , Andreas Dilger , Andrew Morton , Christoph Hellwig , Dan Williams , Dave Chinner , Jan Kara , Matthew Wilcox , linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-nvdimm@lists.01.org, linux-xfs@vger.kernel.org To: Ross Zwisler Return-path: Content-Disposition: inline In-Reply-To: <1475874544-24842-10-git-send-email-ross.zwisler@linux.intel.com> Sender: owner-linux-mm@kvack.org List-Id: linux-ext4.vger.kernel.org On Fri 07-10-16 15:08:56, Ross Zwisler wrote: > DAX radix tree locking currently locks entries based on the unique > combination of the 'mapping' pointer and the pgoff_t 'index' for the entry. > This works for PTEs, but as we move to PMDs we will need to have all the > offsets within the range covered by the PMD to map to the same bit lock. > To accomplish this, for ranges covered by a PMD entry we will instead lock > based on the page offset of the beginning of the PMD entry. The 'mapping' > pointer is still used in the same way. > > Signed-off-by: Ross Zwisler The patch looks good to me. You can add: Reviewed-by: Jan Kara Just one thing which IMO deserves a comment below: > @@ -448,9 +460,12 @@ restart: > } > > void dax_wake_mapping_entry_waiter(struct address_space *mapping, > - pgoff_t index, bool wake_all) > + pgoff_t index, void *entry, bool wake_all) > { > - wait_queue_head_t *wq = dax_entry_waitqueue(mapping, index); > + struct exceptional_entry_key key; > + wait_queue_head_t *wq; > + > + wq = dax_entry_waitqueue(mapping, index, entry, &key); So I believe we should comment above this function that the 'entry' it gets may be invalid by the time it gets it (we call it without tree_lock held so the passed entry may be changed in the radix tree as we work) but we use it only to find appropriate waitqueue where tasks sleep waiting for that old entry to unlock so we indeed wake up all tasks we need. Honza -- Jan Kara SUSE Labs, CR -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 119641A1EC3 for ; Tue, 11 Oct 2016 11:42:32 -0700 (PDT) Date: Tue, 11 Oct 2016 09:04:09 +0200 From: Jan Kara Subject: Re: [PATCH v5 09/17] dax: coordinate locking for offsets in PMD range Message-ID: <20161011070409.GC6952@quack2.suse.cz> References: <1475874544-24842-1-git-send-email-ross.zwisler@linux.intel.com> <1475874544-24842-10-git-send-email-ross.zwisler@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1475874544-24842-10-git-send-email-ross.zwisler@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: Ross Zwisler Cc: Theodore Ts'o , Matthew Wilcox , Dave Chinner , linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-mm@kvack.org, Andreas Dilger , Alexander Viro , Jan Kara , linux-fsdevel@vger.kernel.org, Andrew Morton , linux-ext4@vger.kernel.org, Christoph Hellwig List-ID: On Fri 07-10-16 15:08:56, Ross Zwisler wrote: > DAX radix tree locking currently locks entries based on the unique > combination of the 'mapping' pointer and the pgoff_t 'index' for the entry. > This works for PTEs, but as we move to PMDs we will need to have all the > offsets within the range covered by the PMD to map to the same bit lock. > To accomplish this, for ranges covered by a PMD entry we will instead lock > based on the page offset of the beginning of the PMD entry. The 'mapping' > pointer is still used in the same way. > > Signed-off-by: Ross Zwisler The patch looks good to me. You can add: Reviewed-by: Jan Kara Just one thing which IMO deserves a comment below: > @@ -448,9 +460,12 @@ restart: > } > > void dax_wake_mapping_entry_waiter(struct address_space *mapping, > - pgoff_t index, bool wake_all) > + pgoff_t index, void *entry, bool wake_all) > { > - wait_queue_head_t *wq = dax_entry_waitqueue(mapping, index); > + struct exceptional_entry_key key; > + wait_queue_head_t *wq; > + > + wq = dax_entry_waitqueue(mapping, index, entry, &key); So I believe we should comment above this function that the 'entry' it gets may be invalid by the time it gets it (we call it without tree_lock held so the passed entry may be changed in the radix tree as we work) but we use it only to find appropriate waitqueue where tasks sleep waiting for that old entry to unlock so we indeed wake up all tasks we need. Honza -- Jan Kara SUSE Labs, CR _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:42810 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753560AbcJKSnj (ORCPT ); Tue, 11 Oct 2016 14:43:39 -0400 Date: Tue, 11 Oct 2016 09:04:09 +0200 From: Jan Kara Subject: Re: [PATCH v5 09/17] dax: coordinate locking for offsets in PMD range Message-ID: <20161011070409.GC6952@quack2.suse.cz> References: <1475874544-24842-1-git-send-email-ross.zwisler@linux.intel.com> <1475874544-24842-10-git-send-email-ross.zwisler@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1475874544-24842-10-git-send-email-ross.zwisler@linux.intel.com> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Ross Zwisler Cc: linux-kernel@vger.kernel.org, Theodore Ts'o , Alexander Viro , Andreas Dilger , Andrew Morton , Christoph Hellwig , Dan Williams , Dave Chinner , Jan Kara , Matthew Wilcox , linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-nvdimm@lists.01.org, linux-xfs@vger.kernel.org On Fri 07-10-16 15:08:56, Ross Zwisler wrote: > DAX radix tree locking currently locks entries based on the unique > combination of the 'mapping' pointer and the pgoff_t 'index' for the entry. > This works for PTEs, but as we move to PMDs we will need to have all the > offsets within the range covered by the PMD to map to the same bit lock. > To accomplish this, for ranges covered by a PMD entry we will instead lock > based on the page offset of the beginning of the PMD entry. The 'mapping' > pointer is still used in the same way. > > Signed-off-by: Ross Zwisler The patch looks good to me. You can add: Reviewed-by: Jan Kara Just one thing which IMO deserves a comment below: > @@ -448,9 +460,12 @@ restart: > } > > void dax_wake_mapping_entry_waiter(struct address_space *mapping, > - pgoff_t index, bool wake_all) > + pgoff_t index, void *entry, bool wake_all) > { > - wait_queue_head_t *wq = dax_entry_waitqueue(mapping, index); > + struct exceptional_entry_key key; > + wait_queue_head_t *wq; > + > + wq = dax_entry_waitqueue(mapping, index, entry, &key); So I believe we should comment above this function that the 'entry' it gets may be invalid by the time it gets it (we call it without tree_lock held so the passed entry may be changed in the radix tree as we work) but we use it only to find appropriate waitqueue where tasks sleep waiting for that old entry to unlock so we indeed wake up all tasks we need. Honza -- Jan Kara SUSE Labs, CR From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754337AbcJKSog (ORCPT ); Tue, 11 Oct 2016 14:44:36 -0400 Received: from mx2.suse.de ([195.135.220.15]:42810 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753560AbcJKSnj (ORCPT ); Tue, 11 Oct 2016 14:43:39 -0400 Date: Tue, 11 Oct 2016 09:04:09 +0200 From: Jan Kara To: Ross Zwisler Cc: linux-kernel@vger.kernel.org, "Theodore Ts'o" , Alexander Viro , Andreas Dilger , Andrew Morton , Christoph Hellwig , Dan Williams , Dave Chinner , Jan Kara , Matthew Wilcox , linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-nvdimm@ml01.01.org, linux-xfs@vger.kernel.org Subject: Re: [PATCH v5 09/17] dax: coordinate locking for offsets in PMD range Message-ID: <20161011070409.GC6952@quack2.suse.cz> References: <1475874544-24842-1-git-send-email-ross.zwisler@linux.intel.com> <1475874544-24842-10-git-send-email-ross.zwisler@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1475874544-24842-10-git-send-email-ross.zwisler@linux.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 07-10-16 15:08:56, Ross Zwisler wrote: > DAX radix tree locking currently locks entries based on the unique > combination of the 'mapping' pointer and the pgoff_t 'index' for the entry. > This works for PTEs, but as we move to PMDs we will need to have all the > offsets within the range covered by the PMD to map to the same bit lock. > To accomplish this, for ranges covered by a PMD entry we will instead lock > based on the page offset of the beginning of the PMD entry. The 'mapping' > pointer is still used in the same way. > > Signed-off-by: Ross Zwisler The patch looks good to me. You can add: Reviewed-by: Jan Kara Just one thing which IMO deserves a comment below: > @@ -448,9 +460,12 @@ restart: > } > > void dax_wake_mapping_entry_waiter(struct address_space *mapping, > - pgoff_t index, bool wake_all) > + pgoff_t index, void *entry, bool wake_all) > { > - wait_queue_head_t *wq = dax_entry_waitqueue(mapping, index); > + struct exceptional_entry_key key; > + wait_queue_head_t *wq; > + > + wq = dax_entry_waitqueue(mapping, index, entry, &key); So I believe we should comment above this function that the 'entry' it gets may be invalid by the time it gets it (we call it without tree_lock held so the passed entry may be changed in the radix tree as we work) but we use it only to find appropriate waitqueue where tasks sleep waiting for that old entry to unlock so we indeed wake up all tasks we need. Honza -- Jan Kara SUSE Labs, CR