From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE, SPF_NONE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 7E9711F463 for ; Thu, 19 Dec 2019 22:27:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726959AbfLSW1z (ORCPT ); Thu, 19 Dec 2019 17:27:55 -0500 Received: from mail-lj1-f195.google.com ([209.85.208.195]:47065 "EHLO mail-lj1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726818AbfLSW1z (ORCPT ); Thu, 19 Dec 2019 17:27:55 -0500 Received: by mail-lj1-f195.google.com with SMTP id m26so5502965ljc.13 for ; Thu, 19 Dec 2019 14:27:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=usp-br.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=yOEYVGnk3GzrN5AI4gRT6vErS9WFYtxYMswtoRkjCrU=; b=o/TkdXu8BtwUntFiX6dMI0t/y0Hqw5HtFP5yc9d1OL80bC2nvmlNG0BOYn1myegKaM CiuL6l4WSRWUxt8/pO29rXObUI6041FkZyNlbgMqeASeXWx4sF5nNiFsXxDmpp+5F4qZ PinH60s5NPIuMGK63rUKOje7+pmlD5Atz+euQd7TA1xx2dPhJcyjzZDbIRrhZmdUyypN Y/S7gNHH3hXXZsXR2pnHoBpsx3RyOdZoFs35rjweL/hedFk+rkJHA/4XWVC8GNX5TPrL Vr9qBlJRmPO/rKsI1Lc+EJBSngfxNehQvcF5Y3VGpb7NgEXNtu1l5F989eMcMR2t9uv7 f9FA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=yOEYVGnk3GzrN5AI4gRT6vErS9WFYtxYMswtoRkjCrU=; b=VhSv6V2SEhb79Gd/mmBkK97epZXkCX3BiS922Bgfsnm12R2TiqBC/LYRqu92VyXtlr sDeC6WYYh5kcjcb7D3l0b8SJ6O65nn30nANgA9I4QlzFyXWbNvL2kYqMh6WuYm4eP/AQ 17a4xNiLjVT/Oh+tVvmsMVW9ri2klIFBT83CR3W70j04R7xxXS7atb8Zmi8U2nDF8hz6 CRE9stcKdcqztuoaYEzDMnD0sIw2ZcKERMFFgBr5iZcclF6sPGSrrhhYIOCC8okcUoyT MDVJm7lsU/rpZ7fzgFr/VxrrVe4uy+AEjxfUsoqmaWhkaS6sNYCSRCbo6IYdu2jeD4Wg G2LA== X-Gm-Message-State: APjAAAUzqNLBeLkT2k8PFUo2cTG7YWafaXB9aO/szqLNZ5nF1gEPgaIL s9MuGuAOEaD5kjRLOFmoQT1f9I5bTr3s1hWYjIZ0CA== X-Google-Smtp-Source: APXvYqx3irqEtsAUA4EHQa/F6LhnBsrfQJnpaRpizpFN6EPoLKLx6our1VcxD6nzXumhVHI314c/0S4w4sKeeRUAVkM= X-Received: by 2002:a05:651c:1194:: with SMTP id w20mr7821735ljo.129.1576794473300; Thu, 19 Dec 2019 14:27:53 -0800 (PST) MIME-Version: 1.0 References: <20191114060134.GB10643@sigill.intra.peff.net> <20191114181552.137071-1-jonathantanmy@google.com> <20191115041215.GB21654@sigill.intra.peff.net> In-Reply-To: <20191115041215.GB21654@sigill.intra.peff.net> From: Matheus Tavares Bernardino Date: Thu, 19 Dec 2019 19:27:42 -0300 Message-ID: Subject: Re: [PATCH v2 05/11] object-store: allow threaded access to object reading To: Jeff King Cc: Jonathan Tan , git , Christian Couder , =?UTF-8?B?0J7Qu9GPINCi0LXQu9C10LbQvdCw0Y8=?= , =?UTF-8?B?Tmd1eeG7hW4gVGjDoWkgTmfhu41jIER1eQ==?= , Junio C Hamano , Jonathan Nieder , Stefan Beller Content-Type: text/plain; charset="UTF-8" Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi, Peff and Jonathan Sorry for the delay in re-rolling this series, it was a very busy end of semester. But I finally completed my degree and had time to get back to it :) I tried the rwlock approach, but there were some subtle difficulties. For example, we should hold the lock in write mode when free()-ing the window, and thus, the lock couldn't be declared inside the struct pack_window. Also, it seemed that protecting the window reading at git_inflate() wouldn't be enough: suppose a thread has just read from a window in git_inflate() (holding the rwlock) and is now waiting for the obj_read_mutex to continue its object reading operation. If another thread (that already has the obj_read_mutex) acquires the rwlock in sequence, it could free() the said window. It might not sound like a problem since the first thread has already finished reading from it. But since a pointer to the window would still be in the first thread's stack as a window cursor, it could be later passed down to use_pack() leading to a segfault. I couldn't come up with a solution for this yet. However, re-inspecting the code, it seemed to me that we might already have a thread-safe mechanism. The window disposal operations (at close_pack_windows() and unuse_one_window()) are only performed if window.inuse_cnt == 0. So as a thread which reads from the window will also previously increment its inuse_cnt, wouldn't the reading operation be already protected? Another concern would be close_pack_fd(), which can close packs even with in-use windows. However, as the mmap docs[1] says: "closing the file descriptor does not unmap the region". Finally, we also considered reprepare_packed_git() as a possible conflicting operation. But the function called by it to handle packfile opening, prepare_pack(), won't reopen already available packs. Therefore, IIUC, it will leave the opened windows intact. So, aren't perhaps the window readings performed outside the obj_read_mutex critical section already thread-safe? Thanks, Matheus [1]: https://linux.die.net/man/2/mmap