[pve-devel] [PATCH storage v3 2/5] Cephfs storage plugin

[Wolfgang Bumiller]

On Tue, Jun 12, 2018 at 02:48:48PM +0200, Thomas Lamprecht wrote:
> On 6/11/18 11:31 AM, Alwin Antreich wrote:
> >  - ability to mount through kernel and fuse client
> >  - allow mount options
> >  - get MONs from ceph config if not in storage.cfg
> >  - allow the use of ceph config with fuse client
> > 
(...)
> > +sub cephfs_is_mounted {
> > +    my ($scfg, $storeid, $mountdata) = @_;
> > +
> > +    my $cmd_option = PVE::CephStorageTools::ceph_connect_option($scfg, $storeid);
> > +    my $configfile = $cmd_option->{ceph_conf};
> > +    my $server = $get_monaddr_list->($scfg, $configfile);
> > +
> > +    my $subdir = $scfg->{subdir} ? $scfg->{subdir} : '/';
> 
> We normally use the following style for this:
> 
> my $subdir = $scfg->{subdir} // '/';

Note the subtle difference - // checks for defined()ness of
$scfg->{subdir} in this case which you were probably lacking, depending
on whether explicit empty paths are allowed...


> > +    my $mountpoint = $scfg->{path};
> > +    my $source = "$server:$subdir";
> > +
> > +    $mountdata = PVE::ProcFSTools::parse_proc_mounts() if !$mountdata;
> > +    return $mountpoint if grep {
> > +	$_->[2] =~ m#^ceph|fuse\.ceph-fuse# &&
> > +	$_->[0] =~ m#^\Q$source\E|ceph-fuse$# &&
> > +	$_->[1] eq $mountpoint
> > +    } @$mountdata;
> > +
> > +    warn "A filesystem is already mounted on $mountpoint\n"
> > +	if grep { $_->[1] eq $mountpoint } @$mountdata;
> > +
> > +    return undef;
> > +}
> > +
(...)
> > +
> > +sub activate_storage {
> > +    my ($class, $storeid, $scfg, $cache) = @_;
> > +
> > +    $cache->{mountdata} = PVE::ProcFSTools::parse_proc_mounts()
> > +	if !$cache->{mountdata};
> > +
> > +    my $path = $scfg->{path};
> > +
> > +    if (!cephfs_is_mounted($scfg, $storeid, $cache->{mountdata})) {
> > +
> > +	# NOTE: only call mkpath when not mounted (avoid hang
> > +	# when cephfs is offline
> > +
> > +	mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir});
> 
> what? I do not understand this, who set's mkdir and why should it
> tell us if cephfs is not mounted? 
> I'd just omit above comment, it's more confusing than helpful, IMO.

It's our directory-based storage option to know whether to create the
path leading up to the storage, as doing so when there's an
intermediate mount-point missing might prevent the mount from
happening. (eg. zfs)
But the comment isn't about this condition, it's about the one 2 lines
further up, so move it above the if, or remove the empty line in
between, to group them together.

Btw., since we're mounting a directory, it would probably be nice to
also allow adding the 'is_mountpoint' condition (particularly the path
based variant is always useful).