From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail06.adl2.internode.on.net ([150.101.137.129]:54894 "EHLO ipmail06.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932120AbdJVX1p (ORCPT ); Sun, 22 Oct 2017 19:27:45 -0400 Date: Mon, 23 Oct 2017 10:27:42 +1100 From: Dave Chinner Subject: Re: [PATCH 00/17] Parent Pointers V3 Message-ID: <20171022232741.GU3666@dastard> References: <1508367333-3237-1-git-send-email-allison.henderson@oracle.com> <20171020224157.GS3666@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Amir Goldstein Cc: Allison Henderson , linux-xfs On Sat, Oct 21, 2017 at 10:34:30AM +0300, Amir Goldstein wrote: > On Sat, Oct 21, 2017 at 1:41 AM, Dave Chinner wrote: > > On Thu, Oct 19, 2017 at 07:11:50AM +0300, Amir Goldstein wrote: > >> On Thu, Oct 19, 2017 at 1:55 AM, Allison Henderson > >> wrote: > >> > Hi all, > >> > > >> > This is the third version of parent pointer attributes for xfs. > >> > I've integrated the suggestions made since v2, mostly moving the > >> > attr buffers in the xfs_attr_log_item to pointers that point to > >> > xfs_attr_item. I've also implementing the recovery routines for > >> > the xfs_attr_log_format. If I missed anything please point it > >> > out. As always, comments and feedback are appreciated. Thank > >> > you! > >> > > >> > >> A minor comment about the cover letter. > >> All designated reviewers must know exactly what "parent pointers" are for, > >> but it could be useful to add some context in the cover letter about the purpose > >> of this work for the sake of other readers on the list. Useful to refer to the > >> upcoming scrub support patches. > >> > >> BTW, not sure if this was mentioned in the previous lifetime of those > >> patches, but parent pointers can be used to implement exportfs operation > >> xfs_fs_fh_to_parent() for "non-connectable" file handles (FILEID_INO32_GEN) > >> and to implement xfs_fs_get_name(), which would make reconnect_path() > >> *much* more efficient. > > > > However, XFS only uses FILEID_INO32_GEN for directories > > because they have known parents. For them, we implement ->get_parent() > > and that means reconnect_path just does ->lookup("..") to find the > > parents and doesn't need anything special. > > > > We use FILEID_INO32_GEN_PARENT for all other types of files to > > encode the ino # + generation of the parent inode into the handle. > > That means for any non-dir file handle, our implemention of > > ->fh_to_parent will get us the parent info as efficiently as > > possible. > > We only encode FILEID_INO32_GEN_PARENT when we are asked for > a "connectable" file handle, which is NOT the case with name_to_handle_at(2) > and is the case with nfsd on when NFS share is exported with subtree_check > options, which AFAIK, is the less common case. > > The question is, when we encode a non-connectable file handle > (FILEID_INO32_GEN), will nfsd benefit from getting a connected file handle > after decode? (result may always be connected if dentry is already in cache). > > > IOWs, parent pointers won't actually speed up filehandle -> > > dentry reconnection on XFS at all because we already encode parent > > pointers into the filehandles that need them.... > > Look at default implementation of ->get_name() in expfs.c and you will > see why I wrote that xfs_fs_get_name() would make reconnect_path() > *much* more efficient, even for directories. If you think it will be a win, then I'm not going to stop you from implementing and benchmarking it to demonstrate how much faster it is. Should be pretty simple to benchmark - stat a whole bunch of files on an NFS client, drop server caches, stat the files again. If it's a win, the server CPU and IO load should be lower, and the client side stat rate should be faster.... Cheers, Dave. -- Dave Chinner david@fromorbit.com