From mboxrd@z Thu Jan  1 00:00:00 1970
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751655AbeAPKPT (ORCPT <rfc822;ralf@linux-mips.org> + 1 other);
        Tue, 16 Jan 2018 05:15:19 -0500
Received: from mail-wm0-f48.google.com ([74.125.82.48]:38545 "EHLO
        mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751455AbeAPKPR (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 16 Jan 2018 05:15:17 -0500
X-Google-Smtp-Source: ACJfBovi9nf+owKZ+8X7AsbtjFELX8ArC9A6pvGfyFl2sgIHRUdf/TQSFCveO9ofWlf7hgLWXqkT6Q==
Subject: Re: [PATCH v2] fs: Add VirtualBox guest shared folder (vboxsf)
 support
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Michael Thayer <michael.thayer@oracle.com>,
        Larry Finger <Larry.Finger@lwfinger.net>,
        Christoph Hellwig <hch@infradead.org>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
References: <20180115175116.4130-1-hdegoede@redhat.com>
 <20180115175116.4130-2-hdegoede@redhat.com>
 <20180115223644.GN13338@ZenIV.linux.org.uk>
From: Hans de Goede <hdegoede@redhat.com>
Message-ID: <02e61d96-75b0-cb29-ecb5-36f91f41695e@redhat.com>
Date: Tue, 16 Jan 2018 11:15:15 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <20180115223644.GN13338@ZenIV.linux.org.uk>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Return-Path: <linux-kernel-owner@vger.kernel.org>

Hi Al,

Thank you for the quick review.

On 15-01-18 23:36, Al Viro wrote:
> * what hch said about mount options.  Get rid of that before the thing
>    goes into mainline.

Ack.

> * all "inode = GET_F_DENTRY(f)->d_inode;" are complete BS; it's called
>    file_inode(f).  Kill GET_F_DENTRY() off and be done with that.

Ok, will fix.

> * your ->rename() can race with ->get_link().  Look at the place where
>    the former reassigns ->path and frees the old value and think what
>    happens if the latter is called just prior to that kfree().
> 
> * the same goes for sf_inode_revalidate() vs. rename().
> 
> * just what happens to ->path of inode when e.g. its grandparent
>    directory is renamed?

Can I summarize all 3 above with: Caching the path is a bad idea and
instead the code should always look up the name from e.g. file->f_path.dentry ?

> * AFAICS, you consider all negative dentries invalid.  Why do you even
>    hash them, then?

I'm afraid I'm not entirely following you. Note I've no experience with fs
code prior to this. Also I'm not the original author of this code, this
code started as part of the out-of-tree kernel modules used by the
VirtualBox guest-additions. I've been working on cleaning these modules up
and then mainlinging them (there are 3 of them, this is the last).

Note I believe that this code is based on the fs/hostfs code.

Can you reword your question keeping my lack of experience wrt fs code
in mind ?

Regards,

Hans