Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752414AbcKRJgO convert rfc822-to-8bit (ORCPT ); Fri, 18 Nov 2016 04:36:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36138 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751139AbcKRJgL (ORCPT ); Fri, 18 Nov 2016 04:36:11 -0500 Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 From: David Howells In-Reply-To: <20161117234047.GE28177@dastard> References: <20161117234047.GE28177@dastard> <147938969703.13574.10295364502230379833.stgit@warthog.procyon.org.uk> <147938970382.13574.11581172952175034619.stgit@warthog.procyon.org.uk> To: Dave Chinner Cc: dhowells@redhat.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <26167.1479461768.1@warthog.procyon.org.uk> Content-Transfer-Encoding: 8BIT Date: Fri, 18 Nov 2016 09:36:08 +0000 Message-ID: <26168.1479461768@warthog.procyon.org.uk> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 18 Nov 2016 09:36:10 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4365 Lines: 99 Dave Chinner wrote: > > (5) Data version number: Could be used by userspace NFS servers [Aneesh > > Kumar]. > > > > Can also be used to modify fill_post_wcc() in NFSD which retrieves > > i_version directly, but has just called vfs_getattr(). It could get > > it from the kstat struct if it used vfs_xgetattr() instead. > > This needs a much clearer name that stx_version as "version" is > entirely ambiguous. e.g. Inodes have internal version numbers to > disambiguate life cycles. and there are versioning filesystems > which have multiple versions of the same file. We've already been through that. I wanted to call it stx_data_version but that got argued down to stx_version. The problem is that what the version number means is entirely filesystem dependent, and it might not just reflect changes in the data. > So if stx_version this is intended to export the internal filesystem > inode change counter (i.e. inode->i_version) then lets call it that: > stx_modification_count. It's clear and unambiguous as to what it > represents, especially as this counter is more than just a "data > modification" counter - inode metadata modifications will also > cause it to change.... I disagree that it's unambiguous. It works like mtime, right? Which wouldn't be of use for certain filesystems. An example of this would be AFS, where it's incremented by 1 each time a write is committed, but is not updated for metadata changes. This is what matters for data caching. > > (13) FS_IOC_GETFLAGS value. These could be translated to BSD's st_flags. > > Note that the Linux IOC flags are a mess and filesystems such as Ext4 > > define flags that aren't in linux/fs.h, so translation in the kernel > > may be a necessity (or, possibly, we provide the filesystem type too). > > And we now also have FS_IOC_FSGETXATTR that extends the flags > and information userspace can get from filesystems. It makes little > sense to now add xstat() and not add everything this interface > also exports... I'm not sure I agree. Stuff like extent sizes and extent hint flags seem like very specialised things that don't belong in the stat interface. The project ID, on the other hand is arguably a good thing to include. But we can always add this later. Note that are also two variants of the fsxattr struct defined in the kernel - though one is a superset of the other. > > Time fields are split into separate seconds and nanoseconds fields to make > > packing easier and the granularities can be queried with the filesystem > > info system call. Note that times will be negative if before 1970; in such > > a case, the nanosecond fields will also be negative if not zero. > > So what happens in ten years time when we want to support > femptosecond resolution in the timestamp interface? We've got to > change everything to 64 bit? Shouldn't we just make everything > timestamp related 64 bit? You really think we're going to have accurate timestamps with a resolution of a millionth of a nanosecond? This means you're going to be doing a 64-bit division every time you want a nanosecond timestamp. Also, violet light has a period of ~1.2fs so your 1fs oscillator might emit UV radiation. > > The bits defined in the stx_attributes field convey information about a > > file, how it is accessed, where it is and what it does. The following > > attributes map to FS_*_FL flags and are the same numerical value: > > Please isolate the new interface flags completely from the FS_*_FL > values. We should not repeat the mistake of tying values derived > from filesystem specific on-disk values to a user interface. Why shouldn't I make a numerical correspondance with at least one set of such flags? I get to define a whole new numberspace and can pick the values I want. I see no particular reason to pick explicitly non-corresponding values where possible. Now, I can agree that the code should say, for example: if (ext4->flag & FS_COMPRESSED_FL) statx.attr |= STATX_ATTR_COMPRESSED; if (ext4->flag & FS_ENCRYPTED_FL) statx.attr |= STATX_ATTR_ENCRYPTED; if (ext4->flag & FS_IMMUTABLE_FL) statx.attr |= STATX_ATTR_IMMUTABLE; ... and that the *compiler* should collapse this to: statx.attr |= ext4->flag & mask; but see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78317 David