linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Miklos Szeredi <miklos@szeredi.hu>
To: Ian Kent <raven@themaw.net>
Cc: David Howells <dhowells@redhat.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Al Viro <viro@zeniv.linux.org.uk>, Karel Zak <kzak@redhat.com>,
	Jeff Layton <jlayton@redhat.com>,
	Miklos Szeredi <mszeredi@redhat.com>,
	Nicolas Dichtel <nicolas.dichtel@6wind.com>,
	Christian Brauner <christian@brauner.io>,
	Lennart Poettering <lennart@poettering.net>,
	Linux API <linux-api@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org,
	LSM <linux-security-module@vger.kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [GIT PULL] Filesystem Information
Date: Tue, 4 Aug 2020 16:36:50 +0200	[thread overview]
Message-ID: <CAJfpegu8omNZ613tLgUY7ukLV131tt7owR+JJ346Kombt79N0A@mail.gmail.com> (raw)
In-Reply-To: <ac1f5e3406abc0af4cd08d818fe920a202a67586.camel@themaw.net>

On Tue, Aug 4, 2020 at 4:15 AM Ian Kent <raven@themaw.net> wrote:
>
> On Mon, 2020-08-03 at 18:42 +0200, Miklos Szeredi wrote:
> > On Mon, Aug 3, 2020 at 5:50 PM David Howells <dhowells@redhat.com>
> > wrote:
> > >
> > > Hi Linus,
> > >
> > > Here's a set of patches that adds a system call, fsinfo(), that
> > > allows
> > > information about the VFS, mount topology, superblock and files to
> > > be
> > > retrieved.
> > >
> > > The patchset is based on top of the mount notifications patchset so
> > > that
> > > the mount notification mechanism can be hooked to provide event
> > > counters
> > > that can be retrieved with fsinfo(), thereby making it a lot faster
> > > to work
> > > out which mounts have changed.
> > >
> > > Note that there was a last minute change requested by Miklós: the
> > > event
> > > counter bits got moved from the mount notification patchset to this
> > > one.
> > > The counters got made atomic_long_t inside the kernel and __u64 in
> > > the
> > > UAPI.  The aggregate changes can be assessed by comparing pre-
> > > change tag,
> > > fsinfo-core-20200724 to the requested pull tag.
> > >
> > > Karel Zak has created preliminary patches that add support to
> > > libmount[*]
> > > and Ian Kent has started working on making systemd use these and
> > > mount
> > > notifications[**].
> >
> > So why are you asking to pull at this stage?
> >
> > Has anyone done a review of the patchset?
>
> I have been working with the patch set as it has evolved for quite a
> while now.
>
> I've been reading the kernel code quite a bit and forwarded questions
> and minor changes to David as they arose.
>
> As for a review, not specifically, but while the series implements a
> rather large change it's surprisingly straight forward to read.
>
> In the time I have been working with it I haven't noticed any problems
> except for those few minor things that I reported to David early on (in
> some cases accompanied by simple patches).
>
> And more recently (obviously) I've been working with the mount
> notifications changes and, from a readability POV, I find it's the
> same as the fsinfo() code.
>
> >
> > I think it's obvious that this API needs more work.  The integration
> > work done by Ian is a good direction, but it's not quite the full
> > validation and review that a complex new API needs.
>
> Maybe but the system call is fundamental to making notifications useful
> and, as I say, after working with it for quite a while I don't fell
> there's missing features (that David hasn't added along the way) and
> have found it provides what's needed for what I'm doing (for mount
> notifications at least).

Apart from the various issues related to the various mount ID's and
their sizes, my general comment is (and was always): why are we adding
a multiplexer for retrieval of mostly unrelated binary structures?

<linux/fsinfo.h> is 345 lines.  This is not a simple and clean API.

A simple and clean replacement API would be:

int get_mount_attribute(int dfd, const char *path, const char
*attr_name, char *value_buf, size_t buf_size, int flags);

No header file needed with dubiously sized binary values.

The only argument was performance, but apart from purely synthetic
microbenchmarks that hasn't been proven to be an issue.

And notice how similar the above interface is to getxattr(), or the
proposed readfile().  Where has the "everything is  a file" philosophy
gone?

I think we already lost that with the xattr API, that should have been
done in a way that fits this philosophy.  But given that we  have "/"
as the only special purpose char in filenames, and even repetitions
are allowed, it's hard to think of a good way to do that.  Pity.

Still I think it would be nice to have a general purpose attribute
retrieval API instead of the multiplicity of binary ioctls, xattrs,
etc.

Is that totally crazy?  Nobody missing the beauty in recently introduced APIs?

Thanks,
Miklos

  reply	other threads:[~2020-08-04 14:39 UTC|newest]

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-03 15:27 [GIT PULL] Mount notifications David Howells
2020-08-03 15:49 ` [GIT PULL] Filesystem Information David Howells
2020-08-03 16:42   ` Miklos Szeredi
2020-08-04  2:15     ` Ian Kent
2020-08-04 14:36       ` Miklos Szeredi [this message]
2020-08-05  1:33         ` Ian Kent
2020-08-05  8:00           ` Miklos Szeredi
2020-08-05 11:13             ` Ian Kent
2020-08-05  8:24         ` file metadata via fs API (was: [GIT PULL] Filesystem Information) Miklos Szeredi
2020-08-11 13:54           ` Miklos Szeredi
2020-08-11 14:08             ` Al Viro
2020-08-11 14:22               ` Miklos Szeredi
2020-08-11 14:31                 ` Al Viro
     [not found]                   ` <CAAgocE07=vVKpQhG+rjEGO=NEBKZ02gjg4TRPxECAc+RKrzn=Q@mail.gmail.com>
2020-08-11 14:36                     ` Al Viro
2020-08-11 14:36                   ` Miklos Szeredi
2020-08-11 14:42                     ` Al Viro
2020-08-11 14:47                       ` Miklos Szeredi
2020-08-11 15:20             ` Linus Torvalds
2020-08-11 15:30               ` Miklos Szeredi
2020-08-11 16:05                 ` Linus Torvalds
2020-08-11 18:49                   ` Miklos Szeredi
2020-08-11 19:31                     ` Lennart Poettering
2020-08-11 19:50                       ` Christian Brauner
2020-08-11 19:39                   ` Christian Brauner
2020-08-12  0:53                     ` Ian Kent
2020-08-11 15:39               ` Andy Lutomirski
2020-08-11 16:17                 ` Casey Schaufler
2020-08-11 16:30                   ` Linus Torvalds
2020-08-11 20:28                   ` Miklos Szeredi
2020-08-11 20:36                     ` Jann Horn
2020-08-11 20:56                       ` Miklos Szeredi
2020-08-11 21:17                         ` Andy Lutomirski
2020-08-11 21:18                         ` Linus Torvalds
2020-08-12  7:23                           ` Miklos Szeredi
2020-08-12 14:39                             ` Al Viro
2020-08-12 14:46                               ` Miklos Szeredi
2020-08-12 15:08                                 ` Al Viro
2020-08-12 15:13                                   ` Miklos Szeredi
2020-08-12 16:33                                     ` Al Viro
2020-08-12 17:16                                       ` Miklos Szeredi
2020-08-12 17:39                                         ` Al Viro
2020-08-12 18:33                                           ` Al Viro
2020-08-12 21:30                                             ` Al Viro
2020-08-18  9:41                                               ` Miklos Szeredi
2020-08-18  9:30                                             ` Miklos Szeredi
2020-08-12 15:22                                   ` David Howells
2020-08-11 21:20                     ` Al Viro
2020-08-11 21:35                     ` Casey Schaufler
2020-08-11 16:05               ` Al Viro
2020-08-11 16:09                 ` Linus Torvalds
2020-08-11 16:39                   ` Al Viro
2020-08-12 10:14               ` Karel Zak
2020-08-12 13:09                 ` Miklos Szeredi
2020-08-12 13:33                 ` David Howells
2020-08-12 13:54                   ` Miklos Szeredi
2020-08-12  0:05             ` David Howells
2020-08-12  7:55               ` Miklos Szeredi
2020-08-12  8:29               ` David Howells
2020-08-12  8:37                 ` Miklos Szeredi
2020-08-12  9:43                   ` file metadata via fs API Steven Whitehouse
2020-08-12 10:04                     ` Miklos Szeredi
2020-08-12 11:28                       ` Karel Zak
2020-08-12 12:43                         ` Miklos Szeredi
2020-08-13  8:52                           ` Karel Zak
2020-08-12 13:06                         ` David Howells
2020-08-13  1:01                           ` Ian Kent
2020-08-12 18:18               ` file metadata via fs API (was: [GIT PULL] Filesystem Information) Linus Torvalds
2020-08-12 19:34                 ` file metadata via fs API Steven Whitehouse
2020-08-12 19:50                   ` Linus Torvalds
2020-08-13  3:44                     ` Ian Kent
2020-08-13 10:36                     ` Karel Zak
2020-08-14  7:58                     ` Lennart Poettering
2020-08-17 11:32                     ` Steven Whitehouse
2020-08-17 17:15                       ` Linus Torvalds
2020-08-17 22:44                         ` Linus Torvalds
2020-08-18 12:50                           ` Miklos Szeredi
2020-08-18 18:51                             ` Linus Torvalds
2020-08-18 20:18                               ` Miklos Szeredi
2020-08-18 20:53                                 ` Linus Torvalds
2020-08-21 13:17                                   ` Miklos Szeredi
2020-08-19  2:29                               ` Al Viro
2020-08-13  3:53                 ` file metadata via fs API (was: [GIT PULL] Filesystem Information) Jeffrey E Altman
2020-08-14 17:05                   ` Linus Torvalds
2020-08-18 15:01                     ` Jeffrey E Altman
2020-08-14  8:06                 ` Lennart Poettering
2020-08-12 13:54             ` David Howells
2020-08-12 14:10               ` Miklos Szeredi
2020-08-12 14:23               ` David Howells
2020-08-03 22:48 ` [GIT PULL] Mount notifications Ian Kent

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJfpegu8omNZ613tLgUY7ukLV131tt7owR+JJ346Kombt79N0A@mail.gmail.com \
    --to=miklos@szeredi.hu \
    --cc=christian@brauner.io \
    --cc=dhowells@redhat.com \
    --cc=jlayton@redhat.com \
    --cc=kzak@redhat.com \
    --cc=lennart@poettering.net \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mszeredi@redhat.com \
    --cc=nicolas.dichtel@6wind.com \
    --cc=raven@themaw.net \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [GIT PULL] Filesystem Information' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).