From owner-svn-src-all@freebsd.org Mon Feb 17 11:43:28 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id DF675256056; Mon, 17 Feb 2020 11:43:28 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 48LhvM5Zplz3F5w; Mon, 17 Feb 2020 11:43:27 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x443.google.com with SMTP id z3so19369273wru.3; Mon, 17 Feb 2020 03:43:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=WrnnKy/DRPm5ub+fcbpUqNnAEmIXus2d91hZAWF/4Xo=; b=Fy1zvQEutqYw6Cs21JbuikeNOTBdI2h0cMxIVd891rzqvZRZ72eAUWx0Affgd6JtJI APtORsMtYI6cEBmjxdp0g2aRHnhNrWU7HjwoWMKhL77CkcLm9TVEMs5uLpBR+YEIQ2Hm EvbnKEg6sVBVHp6ByMBxkfZgIbaaOCRMbW7JadOqIBCzT8y6cnBcYV/qVEDpcgd5MW5m Ts8LsnhiqroN0qmmaNdX3+WvcC0D5g0BCYsAoighQ6cyOGLdT1FrJy2WiIhWWu7Kwcm7 sKWqXRnJ+ue5a8OJwG7MdL0TZFlK0I1tHDw1sThpo8KSr/e5NVrB+rSRWq7fj4PPVt5R vi5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=WrnnKy/DRPm5ub+fcbpUqNnAEmIXus2d91hZAWF/4Xo=; b=pHvTYS0e+pmIWN8R3K3o3uM/kvj3w9vnnw27CEtx72jXD1XKWY4jY7u0qv2BhPiGNs p0bai68HKAvT67DdjA3p1bsN3yu3p0Oul+EdyOS2Sx03wuvqimS5sqzfbwCVFhHIhPR0 DKz8CED6tQua4ZERCBdwAIYy7HMxDjvVlxwC9IZ2CblF2mTYK1It+BtRJNSlGRmMbn2P Fh06IpB/vrlUtpSYWfsS30xXsipGA5sa7RPOWDY14OfpsTn2VHJzW0M36Gy+KEtk5AoF lpmvoQWWdmdk+0UkkJmc6IVUIkxf9FSgOEQPINm9dkizuYL/1OmhjEruysbxy/8rfU0g zFLA== X-Gm-Message-State: APjAAAVpQEnMYroWArpje9dO8u+7Rp93B6adxupCD48Ht4aNAMjfawJU emP7AWPF0DxMy14sx3TkY46xb8G47zF+/8jCQUC5uw== X-Google-Smtp-Source: APXvYqxptkKdrqxGQ7HLzdRkvsRUlc7SSe1R9vUxUgZeVUsRtqX4TlAXv6c/1MS/Z3j5godCb7vSXMmS8YrjMtFboM8= X-Received: by 2002:a5d:4651:: with SMTP id j17mr22122716wrs.237.1581939804222; Mon, 17 Feb 2020 03:43:24 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a5d:6b02:0:0:0:0:0 with HTTP; Mon, 17 Feb 2020 03:43:23 -0800 (PST) In-Reply-To: <41588a27-4a53-485f-6cd4-2a4a5b00d94a@FreeBSD.org> References: <202002010646.0116ktUk057327@repo.freebsd.org> <94dd2422-307b-9c06-ad84-d13d2e8a9fa4@FreeBSD.org> <41588a27-4a53-485f-6cd4-2a4a5b00d94a@FreeBSD.org> From: Mateusz Guzik Date: Mon, 17 Feb 2020 12:43:23 +0100 Message-ID: Subject: Re: svn commit: r357361 - in head/sys: kern sys ufs/ufs vm To: John Baldwin Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 48LhvM5Zplz3F5w X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20161025 header.b=Fy1zvQEu; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2a00:1450:4864:20::443 as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-3.00 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20161025]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; R_SPF_ALLOW(-0.20)[+ip6:2a00:1450:4000::/36]; FREEMAIL_FROM(0.00)[gmail.com]; MIME_GOOD(-0.10)[text/plain]; TO_MATCH_ENVRCPT_ALL(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; TO_DN_SOME(0.00)[]; IP_SCORE_FREEMAIL(0.00)[]; IP_SCORE(0.00)[ip: (2.38), ipnet: 2a00:1450::/32(-2.42), asn: 15169(-1.68), country: US(-0.05)]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[3.4.4.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.5.4.1.0.0.a.2.list.dnswl.org : 127.0.5.0]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; RCVD_TLS_ALL(0.00)[]; DWL_DNSWL_NONE(0.00)[gmail.com.dwl.dnswl.org : 127.0.5.0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Feb 2020 11:43:29 -0000 On 2/11/20, John Baldwin wrote: > On 2/10/20 11:54 AM, Mateusz Guzik wrote: >> On 2/3/20, John Baldwin wrote: >>> On 1/31/20 10:46 PM, Mateusz Guzik wrote: >>>> Author: mjg >>>> Date: Sat Feb 1 06:46:55 2020 >>>> New Revision: 357361 >>>> URL: https://svnweb.freebsd.org/changeset/base/357361 >>>> >>>> Log: >>>> vfs: replace VOP_MARKATIME with VOP_MMAPPED >>>> >>>> The routine is only provided by ufs and is only used on mmap and >>>> exec. >>>> >>>> Reviewed by: kib >>>> Differential Revision: https://reviews.freebsd.org/D23422 >>>> >>>> Modified: >>>> head/sys/kern/kern_exec.c >>>> head/sys/kern/vfs_subr.c >>>> head/sys/kern/vnode_if.src >>>> head/sys/sys/vnode.h >>>> head/sys/ufs/ufs/ufs_vnops.c >>>> head/sys/vm/vm_mmap.c >>>> >>>> Modified: head/sys/ufs/ufs/ufs_vnops.c >>>> ============================================================================== >>>> --- head/sys/ufs/ufs/ufs_vnops.c Sat Feb 1 06:41:44 2020 (r357360) >>>> +++ head/sys/ufs/ufs/ufs_vnops.c Sat Feb 1 06:46:55 2020 (r357361) >>>> @@ -108,7 +108,7 @@ static vop_getattr_t ufs_getattr; >>>> static vop_ioctl_t ufs_ioctl; >>>> static vop_link_t ufs_link; >>>> static int ufs_makeinode(int mode, struct vnode *, struct vnode **, >>>> struct componentname *, const char *); >>>> -static vop_markatime_t ufs_markatime; >>>> +static vop_mmapped_t ufs_mmapped; >>>> static vop_mkdir_t ufs_mkdir; >>>> static vop_mknod_t ufs_mknod; >>>> static vop_open_t ufs_open; >>>> @@ -676,19 +676,22 @@ out: >>>> } >>>> #endif /* UFS_ACL */ >>>> >>>> -/* >>>> - * Mark this file's access time for update for vfs_mark_atime(). This >>>> - * is called from execve() and mmap(). >>>> - */ >>> >>> Why remove this comment rather than update it? It is largely still >>> true and explains the purpose of the VOP (update the atime) which is >>> now no longer obvious from the name. >>> >> >> I don't think a fs-specific implementation of a VOP is the right place to >> state where it is called from. I would argue the name could be better as >> the execve bit is definitely not obvious, but interested parties can >> always grep. > > This (always grep) is why comments matter. If we wanted people to just use > grep and always UTSL, we wouldn't bother having any comments at all. One > of the purposes of comments is to allow a human reader to understand the > code > in context without needing several different windows open to piece together > what is happening. In particular, the comment gives a better hint as to > _why_ we are updating the atime. The old name did a better job of this > than > the new one which is more bland, but presumably you have future changes to > add that are beyond just changing the atime and that is why you renamed it? > But this is an example of something which can be grepped for very easily. A comment who is calling given routine is warranted if there is only one such user or there are very special circumstances for it. As it is, should there be another user, the removed comment was prone to getting out of date and if anything was making things more confusing -- why is mark atime routine only called from mmap and exec, when there are so many other ways to trigger atime update? In my opinion comments are there to explain thing which are not obvious or not immediately resolvable (e.g., with said grep). Easy example would be what data is protected with what mechanism, or what can be racing with what. I think my recent additions to vput & friends are a decent example of what in my opinion warrants a comment. -- Mateusz Guzik