From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 45C80C433DF for ; Fri, 15 May 2020 05:33:38 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 15B912065F for ; Fri, 15 May 2020 05:33:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 15B912065F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jZSy5-0002ZM-PG; Fri, 15 May 2020 05:33:09 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jZSy4-0002ZH-M8 for xen-devel@lists.xenproject.org; Fri, 15 May 2020 05:33:08 +0000 X-Inumbo-ID: 8e6dfba4-966d-11ea-9887-bc764e2007e4 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 8e6dfba4-966d-11ea-9887-bc764e2007e4; Fri, 15 May 2020 05:33:07 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 287D2AEED; Fri, 15 May 2020 05:33:09 +0000 (UTC) Subject: Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support From: =?UTF-8?B?SsO8cmdlbiBHcm/Dnw==?= To: Jan Beulich References: <20200508153421.24525-1-jgross@suse.com> <20200508153421.24525-5-jgross@suse.com> <23938228-e947-fe36-8b19-0e89886db9ac@suse.com> Message-ID: <28dd8109-1815-70cd-834c-53330d5c824d@suse.com> Date: Fri, 15 May 2020 07:33:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <23938228-e947-fe36-8b19-0e89886db9ac@suse.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Stefano Stabellini , Julien Grall , Wei Liu , Andrew Cooper , Ian Jackson , George Dunlap , xen-devel@lists.xenproject.org, Daniel De Graaf , Volodymyr Babchuk , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On 14.05.20 11:50, Jürgen Groß wrote: > On 14.05.20 09:59, Jan Beulich wrote: >> On 08.05.2020 17:34, Juergen Gross wrote: >>> +int hypfs_read_dir(const struct hypfs_entry *entry, >>> +                   XEN_GUEST_HANDLE_PARAM(void) uaddr) >>> +{ >>> +    const struct hypfs_entry_dir *d; >>> +    const struct hypfs_entry *e; >>> +    unsigned int size = entry->size; >>> + >>> +    d = container_of(entry, const struct hypfs_entry_dir, e); >>> + >>> +    list_for_each_entry ( e, &d->dirlist, list ) >> >> This function, in particular because of being non-static, makes >> me wonder how, with add_entry() taking a lock, it can be safe >> without any locking. Initially I thought the justification might >> be because all adding of entries is an init-time-only thing, but >> various involved functions aren't marked __init (and it is at >> least not implausible that down the road we might see new >> entries getting added during certain hotplug operations). >> >> I do realize that do_hypfs_op() takes the necessary read lock, >> but then you're still building on the assumption that the >> function is reachable through only that code path, despite >> being non-static. An ASSERT() here would be the minimum I guess, >> but with read locks now being recursive I don't see why you >> couldn't read-lock here again. > > Right, will add the read-lock. > >> >> The same goes for other non-static functions, albeit things may >> become more interesting for functions living on the >> XEN_HYPFS_OP_write_contents path (because write locks aren't > > Adding an ASSERT() in this regard should be rather easy. As the type specific read- and write-functions should only be called through the generic read/write functions I think it is better to have a percpu variable holding the current locking state and ASSERT() that to match. This will be cheaper than nesting locks and I don't have to worry about either write_lock nesting or making _is_write_locked_by_me() an official interface. Juergen