Date: Mon, 17 Feb 2020 12:43:23 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r357361 - in head/sys: kern sys ufs/ufs vm Message-ID: <CAGudoHFBX%2B=wQRGgNt7YD7%2B6Bn058QjyZ0ABZfiUT0_Mc5AzXg@mail.gmail.com> In-Reply-To: <41588a27-4a53-485f-6cd4-2a4a5b00d94a@FreeBSD.org> References: <202002010646.0116ktUk057327@repo.freebsd.org> <94dd2422-307b-9c06-ad84-d13d2e8a9fa4@FreeBSD.org> <CAGudoHHYBzn6mQzdUzuy7zQfh7D60JQqQ=TcFaRFn7i-cR76Sw@mail.gmail.com> <41588a27-4a53-485f-6cd4-2a4a5b00d94a@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2/11/20, John Baldwin <jhb@freebsd.org> wrote: > On 2/10/20 11:54 AM, Mateusz Guzik wrote: >> On 2/3/20, John Baldwin <jhb@freebsd.org> 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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHFBX%2B=wQRGgNt7YD7%2B6Bn058QjyZ0ABZfiUT0_Mc5AzXg>