Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <f3555e3c-06d9-4d19-d3a2-9a2779937e83@ddn.com>
Date:   Thu, 19 May 2022 19:41:59 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create
Content-Language: en-US
To:     Miklos Szeredi <miklos@szeredi.hu>,
        Dharmendra Singh <dharamhans87@gmail.com>
Cc:     Vivek Goyal <vgoyal@redhat.com>, linux-fsdevel@vger.kernel.org,
        fuse-devel <fuse-devel@lists.sourceforge.net>,
        linux-kernel@vger.kernel.org
References: <20220517100744.26849-1-dharamhans87@gmail.com>
 <CAJfpegsDxsMsyfP4a_5H1q91xFtwcEdu9-WBnzWKwjUSrPNdmw@mail.gmail.com>
From:   Bernd Schubert <bschubert@ddn.com>
In-Reply-To: <CAJfpegsDxsMsyfP4a_5H1q91xFtwcEdu9-WBnzWKwjUSrPNdmw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N2lBbWt4T0I3SmtIMVdDcm5SRlQwdXI4OUVKemlqMS8rNVBNc1dRNDlpNTZE?=
 =?utf-8?B?Vno5MEErZzQvU2Z0VWFVV0ljVDEvVE03akpRVDkySVYrb0RVUkhGeXp0T0NC?=
 =?utf-8?B?UkV6RVFNaDFmRWhtU0JCWDcweUFZQkY3czR5WTNzd1g1STFHaTM2M1dMc3Zq?=
 =?utf-8?B?MFBPWjd3UmdiUGpTK3ZCWUtheS9STzN2UlptS0E1SmNHbjl0NWFyTGpCcW1N?=
 =?utf-8?B?ZUtjdmMvdy8vY0ZlWjZYUHpmaUVEdkw3WnViaHlaamxYNXdUcld1V0VTZGxE?=
 =?utf-8?B?Uy9DcldseFk4SXl0NzFLWFVjd0xNNEFEcVo0WWpiY2VjOXFNenRlejV2UllL?=
 =?utf-8?B?R2RvRTBXN3E3S3hZUjk0bTBoZmFmTEI3RFA2TnJlRkdQWHVQUjhxNDF0TVFQ?=
 =?utf-8?B?clk3b1VjOHRGWkk0bUxuSjhBUmhBc1JhY2xNTnBtZFoxVkkxdEFab3NZUTR4?=
 =?utf-8?B?emVCeE1qOHJrOXZWM2thQjhlekZpMkhjdWQ0Z3liR1lhazNFcjRsa2lydHVF?=
 =?utf-8?B?WlJPbHNPTXlDQ0plaDhzdXoxalZSQm9hMVNHQ21zSkpmN05rcUd6WnRGR3Zm?=
 =?utf-8?B?dm1HVWMxc1o1Z2FNbDZRRmJROVVsRzdVNEFmc1J1WGc1c2JudnRiMUoxYi9S?=
 =?utf-8?B?NGFSTis2RXdJSEV4bzQxL1RWMHZKVitsdVAxZVgzOFhoa1loYkI5ZTdtMzl6?=
 =?utf-8?B?bGpDaVNtdktSemNXSis2K2pIOHJ5bWJuWGk4d25lbnRURnlRUUdRL2F5aVY3?=
 =?utf-8?B?WDd1TU5PNnJuc0s2KzRwYnVpQ3FTbG8yOWh2QkdmQUxUVW56THQ1d2d5NzB1?=
 =?utf-8?B?SjB4eHI5QmNrb0RvWHlQelZGVUhGcGFyZm5peUl5c21SbE5Qd3lzU1JaM3ls?=
 =?utf-8?B?UEZIRUpxS0FhTTExYnlIQS93cW9jTjJzeHpUdlBEOEYxWWpsZEZ4cXkvdUcx?=
 =?utf-8?B?RncwUTZWbkpQSk9menJPMGxkZ2JxUEdEbmJFYWRTMmNaNCtzUUt2TkIybXUr?=
 =?utf-8?B?UGxtbVJoUHJTYUpjVXJHd2NXa1VCRzVMK1BIZVUvTHgxaDUyMkg4SGdHckRw?=
 =?utf-8?B?RnNlS0Rlb3RqM1RCUTl1N1VQbXdDVVUxUDBKT1F0dWt4OWdQcjNya1ZwaEg4?=
 =?utf-8?B?WTR6aXFDL3ZwK05iUVZENG4rdnpRbTRUL0owcmMzWHJXK1ZVY2QvUGZ2amZ1?=
 =?utf-8?B?UGZDVTl4S3RjQ0dDbmZBK200eXIrOFhBNU9NMDVpZ2lhdkxuSm9IbVJnTitV?=
 =?utf-8?B?S05kNGk3NXFaZzl0WDBMM3JZQ2xUQWhuL0VsdlpZWkVmT0prK0hNN0Y0VkhJ?=
 =?utf-8?B?UkZoVVlLMHltZit2LzRpUDFvSXVaR1BUVDlKSktSemZrNjQ0SEtDVzlJOVZa?=
 =?utf-8?B?ZlM4YVo2Q0hBZ3pjRUgydktVdlR0MlIwR2Z4TERYZS9SQm9aTXhlWWJZaU81?=
 =?utf-8?B?a2g1ZlpINS9BYUNpejlQL3daV3p2T1hwRWpKbDNubWpCWHo5eXR4SFZTNDJD?=
 =?utf-8?B?TXlPK29LQWVFdUpyS1JpTGMvVGNsUzNvZENMWm5BUFQxZ1JrWkRUR0kvQUt0?=
 =?utf-8?B?N1FJSjZRVjJTalZSR1FVQWV1OFFlMER6TXF6aTRxbGh1bmdIaFlsZE5UMUd1?=
 =?utf-8?B?cEdlU2c2WCtTZ3BzR0VSNXBPbEgrdVlDVXRCVzNidWZab0V2cWdtbXFYZEJC?=
 =?utf-8?B?UVBNSVlnT3NaY1BLbS9QY3U1SVRuSCs3SGN3VDlZa09zaVlZTUNYOEZabHlV?=
 =?utf-8?B?THhtTTVnaDdNTlcxRVI0OVhsNWlkQTVJT1BJSkp1Qmo5M0R1TDJyTkVlMXVa?=
 =?utf-8?B?NVRVNVRFL1B3Q0lzckxoSXlOYVFYRldwckVZQS9nbGlldVNiRkVJLzhEVHVV?=
 =?utf-8?B?NWxKaTdyemY4bzA4Z0VGbVZWWG9KZzVEOGk1QXptUzRDcksyN2dtVlkxRHVo?=
 =?utf-8?B?Tld6cVM0K3lsQzhtZXBWVTlXS3cyL1lzYjdBODhlWmhUNitRaWlmQ01RRW5X?=
 =?utf-8?B?SXZRdjk0NGR6MnhJa3JCRk8wTFBkMkxqdThMZS82ZzFubkphMWVGTG9oVkFu?=
 =?utf-8?B?SUpJcTlJNXZick5yNTRISXZNRlJ5aUFQSnhpRi9kN3hrOWdLZW5EbFlWVGla?=
 =?utf-8?B?bW5qOHp6eXJXdVJ3MzUxajI3YTB0blh3NUpYRmQ4Q2gyampTVDlKUlliL2Q1?=
 =?utf-8?B?ZlVOaHYzNVFnaTRCSklYejRKVlJtNTFCUVJzcUJ5bmd2RHFlRzc0NEFTSXNV?=
 =?utf-8?B?WStRMXBLMjJ5K0xnU3pFNy9IbERBaWh5dEZQV2pxN21uMjBuYXBuNmt3bFBB?=
 =?utf-8?B?dklpRE56ZVRpSnFxNi9vQm5lY0dvM3VxZjYwek9ITVBHZlR3ZFc1UT09?=
X-OriginatorOrg: ddn.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a0363e51-57c9-4311-80b6-08da39bee320
X-MS-Exchange-CrossTenant-AuthSource: DM5PR1901MB2037.namprd19.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 May 2022 17:42:04.6286
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 753b6e26-6fd3-43e6-8248-3f1735d59bb4
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PUSb+Eigsyb1ebUf+H4NqLrcthdA8EMb49GM8pJe7hU5xTFxRgQEapSy6GXyx5F4Ii68a/sadoo8VgKHJ81P3g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR19MB1275
X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 5/19/22 11:39, Miklos Szeredi wrote:
> On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
>>
>> In FUSE, as of now, uncached lookups are expensive over the wire.
>> E.g additional latencies and stressing (meta data) servers from
>> thousands of clients. These lookup calls possibly can be avoided
>> in some cases. Incoming three patches address this issue.
>>
>>
>> Fist patch handles the case where we are creating a file with O_CREAT.
>> Before we go for file creation, we do a lookup on the file which is most
>> likely non-existent. After this lookup is done, we again go into libfuse
>> to create file. Such lookups where file is most likely non-existent, can
>> be avoided.
> 
> I'd really like to see a bit wider picture...
> 
> We have several cases, first of all let's look at plain O_CREAT
> without O_EXCL (assume that there were no changes since the last
> lookup for simplicity):
> 
> [not cached, negative]
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 
> [not cached, positive]
>     ->atomic_open()
>        LOOKUP
>     ->open()
>        OPEN
> 
> [cached, negative, validity timeout not expired]
>     ->d_revalidate()
>        return 1
>     ->atomic_open()
>        CREATE
> 
> [cached, negative, validity timeout expired]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 
> [cached, positive, validity timeout not expired]
>     ->d_revalidate()
>        return 1
>     ->open()
>        OPEN
> 
> [cached, positive, validity timeout expired]
>     ->d_revalidate()
>        LOOKUP
>        return 1
>     ->open()
>        OPEN
> 
> (Caveat emptor: I'm just looking at the code and haven't actually
> tested what happens.)
> 
> Apparently in all of these cases we are doing at least one request, so
> it would make sense to make them uniform:
> 
> [not cached]
>     ->atomic_open()
>        CREATE_EXT
> 
> [cached]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        CREATE_EXT
> 
> Similarly we can look at the current O_CREAT | O_EXCL cases:
> 
> [not cached, negative]
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 
> [not cached, positive]
>     ->atomic_open()
>        LOOKUP
>     return -EEXIST
> 
> [cached, negative]
>     ->d_revalidate()
>        return 0 (see LOOKUP_EXCL check)
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 
> [cached, positive]
>     ->d_revalidate()
>        LOOKUP
>        return 1
>     return -EEXIST
> 
> Again we are doing at least one request, so we can unconditionally
> replace them with CREATE_EXT like the non-O_EXCL case.
> 
> 
>>
>> Second patch handles the case where we open first time a file/dir
>> but do a lookup first on it. After lookup is performed we make another
>> call into libfuse to open the file. Now these two separate calls into
>> libfuse can be combined and performed as a single call into libfuse.
> 
> And here's my analysis:
> 
> [not cached, negative]
>     ->lookup()
>        LOOKUP
>     return -ENOENT
> 
> [not cached, positive]
>     ->lookup()
>        LOOKUP
>     ->open()
>        OPEN
> 
> [cached, negative, validity timeout not expired]
>      ->d_revalidate()
>         return 1
>      return -ENOENT
> 
> [cached, negative, validity timeout expired]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        LOOKUP
>     return -ENOENT
> 
> [cached, positive, validity timeout not expired]
>     ->d_revalidate()
>        return 1
>     ->open()
>        OPEN
> 
> [cached, positive, validity timeout expired]
>     ->d_revalidate()
>        LOOKUP
>        return 1
>     ->open()
>        OPEN
> 
> There's one case were no request is sent:  a valid cached negative
> dentry.   Possibly we can also make this uniform, e.g.:
> 
> [not cached]
>     ->atomic_open()
>         OPEN_ATOMIC
> 
> [cached, negative, validity timeout not expired]
>      ->d_revalidate()
>         return 1
>      return -ENOENT
> 
> [cached, negative, validity timeout expired]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        OPEN_ATOMIC
> 
> [cached, positive]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        OPEN_ATOMIC
> 
> It may even make the code simpler to clearly separate the cases where
> the atomic variants are supported and when not.  I'd also consider
> merging CREATE_EXT into OPEN_ATOMIC, since a filesystem implementing
> one will highly likely want to implement the other as well.


Can you help me a bit to understand what we should change? I had also 
already thought to merge CREATE_EXT and OPEN_ATOMIC - so agreed.
Shall we make the other cases more visible?

Also thanks a lot for you revalidate patch.


Thanks,
Bernd

Thanks,
Bernd