Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jeff Layton <jlayton@poochiereds.net>,
	"J . Bruce Fields" <bfields@fieldses.org>,
	overlayfs <linux-unionfs@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v2 04/17] ovl: decode connected upper dir file handles
Date: Fri, 5 Jan 2018 14:33:22 +0200	[thread overview]
Message-ID: <CAOQ4uxhpNCUzcuFSvntAN_JDB=L5NDqZP4m9TxytyO-P6t3F0Q@mail.gmail.com> (raw)
In-Reply-To: <1515086449-26563-5-git-send-email-amir73il@gmail.com>

On Thu, Jan 4, 2018 at 7:20 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Until this change, we decoded upper file handles by instantiating an
> overlay dentry from the real upper dentry. This is sufficient to handle
> pure upper files, but insufficient to handle merge/impure dirs.
>
> To that end, if decoded real upper dir is connected and hashed, we
> lookup an overlay dentry with the same path as the real upper dir.
> If decoded real upper is non-dir, we instantiate a disconnected overlay
> dentry as before this change.
>
> Because ovl_fh_to_dentry() returns connected overlay dir dentries,
> exportfs never need to call get_parent() and get_name() to reconnect an
> upper overlay dir. Because connectable non-dir file handles are not
> supported, exportfs will not be able to use fh_to_parent() and get_name()
> methods to reconnect a disconnected non-dir to its parent. Therefore, the
> methods get_parent() and get_name() are implemented just to print out a
> sanity warning and the method fh_to_parent() is implemented to warn the
> user that using the 'subtree_check' exportfs option is not supported.
>

Reviewers who will get this far, should have their eyebrows slightly raised
after reading this commit message and should be asking themselves:

"Why not return a disconnected overlay dentry like any other fs and implement
ovl_get_parent()/ovl_get_name() by looking at parent/name of upper dir?"

I have had this debate with myself for a while and experimented a bit with
both approaches and in the end, I liked the "return connected dentry" result
better. I did not want to write this entire story in commit message, because
in the end, there is nothing incorrect about the choice of either implementation
there are only pros and cons to each choice.

At the moment, the only argument I can think of to counter the chosen approach
is that it adds ~100 lines on code in ovl_lookup_real() and
ovl_lookup_real_one()
helpers that could have been avoided by using the common reconnect_path()
code in fs/exportfs/expfs.c.

The arguments to counter the disconnected dir approach are:
- Obtaining a disconnected overlay dir dentry would requires a
delicate re-factoring
  of ovl_lookup() to get a dentry with overlay parent info. I
personally preferred to
  avoid doing that re-factoring unless it was proven worthy.
- Going down the path of disconnected dir would mean that the (non trivial) code
  path of d_splice_alias() could be traveled and that meant writing
more tests and
  introduces race cases that are very hard to hit on purpose. Taking the path of
  connecting overlay dentry by forward lookup is therefore the safe and boring
  way to avoid surprises.
- In the current implementation, there is an anomaly in the multi
lower layer setup.
  In that case, indexed upper dir inodes are hashed by the lower
inode, but their file
  handles are encoded from the upper inode. Obtaining a disconnected
dir from this
  type of upper file handle would have been a special case that would add more
  code and more complexity. With the forward lookup connect approach, the
  anomaly does not require changing the code - connecting the dentry
is just less
  efficient in case there is an ancestor in inode cache (we won't find
it in cache
  because we will be looking with the wrong inode) and that can be
fixed later if
  we find that use case important enough.

There. Now you see why I did not want this story in commit message?

Amir.

  reply	other threads:[~2018-01-05 12:33 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-04 17:20 [PATCH v2 00/17] Overlayfs NFS export support Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 01/17] ovl: document NFS export Amir Goldstein
2018-01-11 16:06   ` Miklos Szeredi
2018-01-11 16:26     ` Amir Goldstein
2018-01-12 15:43       ` Miklos Szeredi
2018-01-12 15:49         ` Miklos Szeredi
2018-01-12 18:50           ` Amir Goldstein
2018-01-13  8:54           ` Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 02/17] ovl: encode pure upper file handles Amir Goldstein
2018-01-18 10:31   ` Miklos Szeredi
2018-01-04 17:20 ` [PATCH v2 03/17] ovl: decode " Amir Goldstein
2018-01-18 14:09   ` Miklos Szeredi
2018-01-18 14:34     ` Amir Goldstein
2018-01-18 14:39       ` Miklos Szeredi
2018-01-18 19:49         ` Amir Goldstein
2018-01-18 20:10           ` Miklos Szeredi
2018-01-18 20:35             ` Amir Goldstein
2018-01-18 22:57               ` Amir Goldstein
2018-01-19  0:23                 ` Amir Goldstein
2018-01-19 10:39                   ` Miklos Szeredi
2018-01-19 11:07                     ` Amir Goldstein
2018-01-19 20:10                       ` Amir Goldstein
2018-01-24 10:34                         ` Miklos Szeredi
2018-01-24 11:04                           ` Amir Goldstein
2018-01-24 11:18                             ` Amir Goldstein
2018-01-24 11:55                               ` Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 04/17] ovl: decode connected upper dir " Amir Goldstein
2018-01-05 12:33   ` Amir Goldstein [this message]
2018-01-05 15:18     ` J . Bruce Fields
2018-01-05 15:34       ` Amir Goldstein
2018-01-15 11:41     ` Miklos Szeredi
2018-01-15 11:33   ` Miklos Szeredi
2018-01-15 12:20     ` Amir Goldstein
2018-01-15 14:56       ` Miklos Szeredi
2018-01-17 11:18         ` Amir Goldstein
2018-01-17 12:20           ` Amir Goldstein
2018-01-17 13:29             ` Amir Goldstein
2018-01-17 15:42           ` Miklos Szeredi
2018-01-17 16:34             ` Amir Goldstein
2018-01-17 21:36               ` Amir Goldstein
2018-01-18  8:22               ` Miklos Szeredi
2018-01-18  8:47                 ` Amir Goldstein
2018-01-18  9:12                   ` Miklos Szeredi
2018-01-18 10:28                     ` Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 05/17] ovl: encode non-indexed upper " Amir Goldstein
2018-01-15 11:58   ` Miklos Szeredi
2018-01-15 12:07     ` Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 06/17] ovl: copy up before encoding dir file handle when ofs->numlower > 1 Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 07/17] ovl: encode lower file handles Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 08/17] ovl: decode lower non-dir " Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 09/17] ovl: decode indexed " Amir Goldstein
2018-01-18 13:11   ` Miklos Szeredi
2018-01-04 17:20 ` [PATCH v2 10/17] ovl: decode lower file handles of unlinked but open files Amir Goldstein
2018-01-16  9:16   ` Miklos Szeredi
2018-01-16  9:37     ` Amir Goldstein
2018-01-16 10:10       ` Miklos Szeredi
2018-01-16 10:40         ` Amir Goldstein
2018-01-16 11:07           ` Miklos Szeredi
2018-01-17 21:05         ` Amir Goldstein
2018-01-18 14:18   ` Amir Goldstein
2018-02-27 11:35     ` Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 11/17] ovl: decode indexed dir file handles Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 12/17] ovl: decode pure lower " Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 13/17] ovl: hash directory inodes for NFS export Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 14/17] ovl: lookup connected ancestor of dir in inode cache Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 15/17] ovl: lookup indexed ancestor of lower dir Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 16/17] ovl: wire up NFS export support Amir Goldstein
2018-01-04 17:20 ` [PATCH v2 17/17] nfsd: encode stat->mtime for getattr instead of inode->i_mtime Amir Goldstein

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOQ4uxhpNCUzcuFSvntAN_JDB=L5NDqZP4m9TxytyO-P6t3F0Q@mail.gmail.com' \
    --to=amir73il@gmail.com \
    --cc=bfields@fieldses.org \
    --cc=jlayton@poochiereds.net \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    --subject='Re: [PATCH v2 04/17] ovl: decode connected upper dir file handles' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).