if we can't get /tmp locally then we need to find some way to create a shared memory region.  posix shared memory or something?

the new file format is more NFS-safe (should be completely safe, in fact)

the only risk now is that multiple writers may attempt an atomic-replace of the file at the same time, resulting in some loss of data.

This (obviously) prevents programs using DConf to run if the home directory of the user is over NFS.
Unity 2D for example encounters that case: https://bugs.launchpad.net/dconf/+bug/817368

The bug mentioned in comment 3 steps from Ubuntu's lack of support for XDG_RUNTIME_DIR, on which dconf depends.  This results in what should be an otherwise unreached codepath being reached, triggering an abort.

I just removed that code with this commit:

commit 44d56d869d2236ef1d177bd1ab33d65ae23d7e2e
Author: Ryan Lortie <desrt@desrt.ca>
Date:   Sun Sep 11 15:09:52 2011 -0400

    Remove "service func" logic
    
    This was required to support the case where the dconf client was unable
    to determine the cache directory for itself.  Since we now use the XDG
    runtime directory, this is redundant.

useful link: http://irccrew.org/~cras/nfs-coding-howto.html

very brief sketch from some past conversation of how this might be improved: 
polling, trying to lock on write, and time-travelling up to minute, seems like a reasonable strawman for how multi-login might be handled

Hi,

I've tested how DConf behave on setup with 2 F16 virtual machines which have home directories shared over NFS. There showed up some problems during that:

1) It is not possible to use dconf watch, it doesn't signal change
caused by the second computer.

2) Sometimes it happens that dconf can not set a value because of this:
** (process:1561): WARNING **:
GDBus.Error:org.gtk.GDBus.UnmappedGError.Quark._g_2dfile_2derror_2dquark.Code24:
Failed to rename file '/home/remote/test/.config/dconf/user.G2KF5V' to
'/home/remote/test/.config/dconf/user': g_rename() failed: Device or
resource busy

Regards

Marek

So, to establish some basics here:
The way dconf works is that clients mmap a cache file that is guaranteed to live in a local fs (/run/user/<username>/dconf/user), and there is a protocol for the daemon to notify clients when it updates the cache file. Only the daemon ever writes the cache file. The same is true for the persistent database, which lives in the users home dir (~/.config/dconf/user). The daemon expects that it is the only one to ever write the persistent database, but it does take care to replace it atomically, and uses fsync when appropriate. 
(All this information should really be in the dconf docs, btw)

The problems you are seeing are due to the 2 daemons
- not seeing each others writes to the persistent db
- not serializing their writes to the persistent db

So, what we need is
a) some protocol for the daemons to serialize their writes
b) code in the daemon that notices changes in the persistent db, and reloads it if it changes

I've prepared another test which consists of 2 applications executed on 2 different virtual machines with shared homes over NFS:

producer:
  - this application just randomly selects which variable to change and sets it to a value which is randomly selected from a shared database
  - it stores names of variables together with indices of values changed to file named 'lock'
  - waits for removal of file 'lock' and repeat the cycle


consumer:
  - waits until there is file 'lock', then it reads its content and compares the values mentioned in it with actual values read from dconf's database
  - removes file 'lock' and waits for new one


Results:
  It fails almost every time. It is probably due to the mechanism which Matthias described in comment #8.
  I can see correct value after stop of consumer by reading the last changed variable with "dconf read".
  I tried it both with newly created DConfClient for each read/write and also with one client for all reads and one client for all writes.
  If I change/read values manually by "dconf write/read" I see the change on the second computer "immediately".

Regards

Marek

P.S.: I've tested this with DConf 0.11.0.

(a) could probably work on the basis of "you cannot replace the file unless you have a write lock on the old one".  there could be an annoying race here, however, between opening the file and acquiring a lock on it:

  client 1                client 2
  open
                          open
                          lock
                          replace
  lock (old file)
  replace again

so perhaps some separate lock file could be helpful.... Ugly.

(b) is probably going to necessarily look like polling in some form or other unless NFS has grown filesystem change notifications...

Created attachment 203009 [details] [review]
serialize writes to db

Hi,

I've prepared a patch for the serialization part. It checks whether the database file is on NFS share and creates lock file for it in such case. If the lock already exists then it waits until the lock file disappear and creates the lock file.
I tried several ways of how to do this and the one in the patch worked best for me.
The only problem here is that if a dconf-service is stopped (crash or ctrl+c) between creation of the lock and deletion of the lock, then the lock file stays there.

Regards

Marek

hi Marek,

A couple of comments about the patch.

This code doesn't belong in gvdb, for a couple of reasons.  The first is that gvdb is too low-level to want to have this kind of locking functionality in it.  The second is that the lock doesn't actually solve any problems in its current form, since you're basically taking this:

{
  read();
  update();
  write();
}

and replacing it with:

{
  read();
  update();
  lock {
    write ();
  }
}

when you should rather have:

{
  lock {
    read();
    update();
    write();
  }
}

in order to avoid data loss.


The second issue is that you should probably use proper fcntl file locking in order to avoid the problem of the state lock file and also to avoid having to sleep/poll in order to attempt acquiring the lock.

Perhaps we need to think a little bit more outside of the box, however.

An interesting question: is it a valid usecase to have a client logged in over NFS and locally at the same time, or is our support for NFS limited to a strict client/server situation where users never log directly on at the NFS server?

If that's the case, then in the NFS situation we could use a different file format entirely -- perhaps an append-only journal with locking and periodic keyframes -- and create a local cache of it in the xdg data dir.

Even if the user wanted to login directly at the server, we could detect the existence of this other file format and act as if we were on NFS anyway.  The caveat of that would be that you'd degrade the performance of your local login by ever logging in over NFS -- even once.

Mixing local and nfs is not an interesting use case as far as we are concerned.
The case we care about it nfs homedirs that are _always_ used via nfs, possibly concurrently from multiple clients.

Hi Ryan,

thank you for your reply.

(In reply to comment #12)
> hi Marek,
> 
> A couple of comments about the patch.
> 
> This code doesn't belong in gvdb, for a couple of reasons.  The first is that
> gvdb is too low-level to want to have this kind of locking functionality in it.
>  The second is that the lock doesn't actually solve any problems in its current
> form, since you're basically taking this:
> 
> {
>   read();
>   update();
>   write();
> }
> 
> and replacing it with:
> 
> {
>   read();
>   update();
>   lock {
>     write ();
>   }
> }

This is actually what I wanted. It fixes this problem:

** (process:1561): WARNING **:
GDBus.Error:org.gtk.GDBus.UnmappedGError.Quark._g_2dfile_2derror_2dquark.Code24:
Failed to rename file '/home/remote/test/.config/dconf/user.G2KF5V' to
'/home/remote/test/.config/dconf/user': g_rename() failed: Device or
resource busy


> when you should rather have:
> 
> {
>   lock {
>     read();
>     update();
>     write();
>   }
> }
> 
> in order to avoid data loss.

Yes, it is a way but where to initiate the lock? In dconf_client_new() ? What
if an application creates a client at its start and finishes it at its end?
This would block other clients.


> The second issue is that you should probably use proper fcntl file locking in
> order to avoid the problem of the state lock file and also to avoid having to
> sleep/poll in order to attempt acquiring the lock.

I wanted to avoid the problem with the race between opening the file and
acquiring a lock on it. But, yes I see a way how to do this with fcntl now.

Marek

If you look in dconf-rebuilder.c you see at the bottom a function called dconf_rebuilder_rebuild() that does the read/update/write.  It would have to be here.

Created attachment 203073 [details] [review]
serialize writes to db 2

(In reply to comment #16)
> If you look in dconf-rebuilder.c you see at the bottom a function called
> dconf_rebuilder_rebuild() that does the read/update/write.  It would have to be
> here.

OK, I understand now. I have rewritten the previous patch to be in dconf_rebuilder_rebuild() and to use fcntl().

But the problem with "g_rename() failed: Device or resource busy" persists. It is probably due to the fact that when we lock the file with fcntl() and it is then removed because of g_rename() in g_file_set_contents() we loose the lock and the other process starts using the file.

Marek

I think you probably need a separate file, just for locking purposes, that does not get replaced/written at all ?

(In reply to comment #18)
> I think you probably need a separate file, just for locking purposes, that does
> not get replaced/written at all ?

Yes, it could help. I'll test it. Thanks.

Created attachment 203958 [details] [review]
notify about remote changes in databases

Hi,

this patch add notify functionality for the case when user database is on a NFS filesystem. It uses polling with interval of 1 minute. It polls only files on NFS. It finds which keys where changed and signalizes it in the similar way to the original 'watch'.
I'll try to send the version with the detached lock file tomorrow.

Regards

Marek

I agree with Matthias about the need to use a separate file for locking.  See comment 10 for an outline of why locking the same filename could be a problem.

As for polling: we should definitely not have each client polling separately.  Rather, we should have the daemon doing a poll and sending change signals in the usual way if it detects changes.

When I change the locking to the detached file then my test timeouts (probably because of slow NFS).
I'll continue on this after new year.

P.S.: Unfortunately I was sick last 2 days so I couldn't continue on this.

no worries -- many people are already on vacation now anyway.

Enjoy the holidays :)

Created attachment 204560 [details] [review]
notify about remote changes in databases - reworked

Thanks :)

> As for polling: we should definitely not have each client polling separately. 
> Rather, we should have the daemon doing a poll and sending change signals in
> the usual way if it detects changes.

I've moved the check for database change to the dconf service. It signalise the change via Notify signal now and writes 0x01 to the database's shm file so the engine rereads the database when client wants the new value.

Created attachment 204561 [details] [review]
notify about remote changes in databases - reworked

Use 60s as timeout for the poll.

I see there is no explicit opt-out, and instead we try to detect if we are on nfs - does this work well in practice ? And does this patch make your tests work ?

(In reply to comment #26)
> I see there is no explicit opt-out, and instead we try to detect if we are on
> nfs - does this work well in practice ?

I'm not sure what you mean by the explicit opt-out here.

> And does this patch make your tests work ?

It works for "dconf watch", although it lasts 1 minute before you see the change. Thinking about it, I've should add check for the file change when reading a value from dconf. Without it it can happen that you read a new value of a key but the watch haven't signalised the change yet. But it won't be easy to decide where to insert the code.

> I'm not sure what you mean by the explicit opt-out here.

Clarified in irc that it is not needed, since you check whether the user db is on an nfs filesystem or not.


> I've should add check for the file change when reading a value from dconf.

Good point.

Created attachment 204744 [details] [review]
serialize writes to db 3

Hi,

this patch creates new lock file and uses it for locking by fcntl(). The name is a concatenation of the database path + '-lock'.
It can happen that one of those concurrent process will occupy the file for quite a long time if it performs a lot of writes to the file but it should work well in usual case.

Regards

Marek

P.S.: I've also tested a modification in which the lock file is unlinked after its use (right before its close) and it was quite fast but we can not be sure that also the database file was already updated (because of NFS).

(In reply to comment #27)
> Thinking about it, I've should add check for the file change when
> reading a value from dconf. Without it it can happen that you read a new value
> of a key but the watch haven't signalised the change yet. But it won't be easy
> to decide where to insert the code.

Unfortunately, the service doesn't know about reads from database. Only DConfClient knows about that. Maybe I've should move the check for changed values back to DConfClient and leave just the file change checker in the service. We could add a new notify signal which would signal database change (e.g. 'DatabaseChangeNotify' or something like that) to the client (and its handler has to emit the standard 'Notify' signal to have watched keys filtered).

What do you think about that?

(In reply to comment #30)
> (In reply to comment #27)
> > Thinking about it, I've should add check for the file change when
> > reading a value from dconf. Without it it can happen that you read a new value
> > of a key but the watch haven't signalised the change yet. But it won't be easy
> > to decide where to insert the code.
> 
> Unfortunately, the service doesn't know about reads from database. Only
> DConfClient knows about that. Maybe I've should move the check for changed
> values back to DConfClient and leave just the file change checker in the
> service. We could add a new notify signal which would signal database change
> (e.g. 'DatabaseChangeNotify' or something like that) to the client (and its
> handler has to emit the standard 'Notify' signal to have watched keys
> filtered).

This doesn't work. The DConfClient which reads the new value doesn't know about the change because it compares mtime which it got at its creation with mtime from the time it reads the value. This means that the client got them almost at the same time for single reads :(. So the client sees no change.

Hi Ryan,

do you think that those two patches for serializing and notifying are ready for commit to dconf's git? I would like to push them there.

Marek

Marek and I just had a chat about this in person.  The conclusion is the following:

 - lock files are good

 - the scanning for changes is also good but maybe not necessary -- can just say
   that possibly all keys have been changed ("/").

 - clients should never stat on a timeout, but maybe the service can do this
   (but only on NFS)

 - we should avoid mmap on NFS at all costs (by using XDG_RUNTIME_DIR)

 - the service can periodically check for changes and synchronise the on-NFS
   copy with the one in the XDG runtime dir

 - when we write, we do a synchronise step first and (while still hold lock)
   do the write (to both DBs).

Created attachment 208498 [details] [review]
add notifications and db locking for dbs on NFS

Hi,

this patch implements conclusions from comment #33.

Locking was moved to service.c so we can lock synchronization of dbs in runtime directory with dbs in config directory together with writes to them in service.c.

It doesn't search for changed keys now, it simply notifies change in "/" for any change of a db on NFS.

The timeout has been moved to service.c. It stats on NFS regularly (every 60s).

It doesn't mmap on NFS now. We just read the copy in runtime directory (it uses g_get_user_runtime_dir() to get XDG_RUNTIME_DIR).

Regards

Marek

Created attachment 210106 [details] [review]
add notifications and db locking for dbs on NFS

I've rebased the patch to adjust for some changes that were made to the engine code; specifically, we now support multiple user databases.

Some initial comments about things I noticed during the rebasing (engine parts only):

 - there appears to be a missing dconf-fs.[ch] which presumably contains dconf_is_on_nfs().

 - no code on the client side ever waits for the service to be started in the on-nfs case.  This is necessary in order to copy the file over in order to ensure the client sees the user's settings

 - the dconf-engine code is looking more and more crufty.  This is a matter of the style in which it is written, which is really my fault.  I plan to clean that up a bit during the next cycle.  That won't impact the vast majority of your changes, though (contained in the service side).

Created attachment 210324 [details] [review]
add notifications and db locking for dbs on NFS

>  - there appears to be a missing dconf-fs.[ch] which presumably contains
> dconf_is_on_nfs().

I'm sorry, I forgot to add them to the patch. Attached is your patch with those files added.

Created attachment 211919 [details] [review]
add notifications and db locking for dbs on NFS

Hi,

I've modified the previous patch so that it pings the DConf service before reading values from its db. I've also changed the test for being on NFS a little. It also returns TRUE if it gets ESTALE error now.

Regards

Marek

Any chance of this getting merged?

Update for those interested: I spent the past little while adding support for backends to dconf-service.

In short, what happens is that on startup, the client kicks the service to start.  The chosen backend then creates a dconf database (gvdb) in the user runtime dir and populates it as it sees fit.  Applications access the database out of the runtime dir, avoiding any worries of mmap-over-NFS.

As of today, I landed initial support for a keyfile backend.  There is no locking or filesystem monitoring (or periodic polling as we would want in NFS) but those will come soon.

I think this should be the preferred backend for people who want to use NFS.  There is really no point in storing the database as GVDB format on an NFS share when the service will just have to open it and sync it up with the GVDB in the runtime directory anyway.  Meanwhile, a keyfile backend could be useful to binary-file-format haters.

One caveat is that this has to be manually configured.  dconf will not attempt to detect that there is an NFS home directory and switch over to this configuration -- you have to manually select it in the profile setup.  That's pretty easy, though.  On each client create '/etc/dconf/profile/user' and put this one line in it:

service-db:keyfile/user


That will result in user settings being stored as a keyfile in ~/.config/dconf-keyfile/user.

FYI: after I add proper locking and monitoring, I will close this bug.  If anyone feels that we should have a backend for GVDB-over-NFS please shout about it now and tell me why it's important for you.

I added the locking and did the monitoring with GFileMonitor.  As of bug 592211 being fixed, we have (something approximating) proper GFileMonitor support on NFS, so this issue is closed.

Hi,

I've finally got to test the NFS support in dconf. Here are some findings I have so far:

It seems that client has to write a value locally (dconf write ...) before it starts to get notifications of changes (dconf watch ...) done on another host. It also doesn't actualize the value before the write hence the host sees old values.
This is related to the creation of GFileMonitor during initialization
of DConfKeyfileWriter. Could we create the GFileMonitor during creation of the watch?

Another thing is the monitoring of gvdb changes on NFS. It would be great if gvdb could use the GFileMonitor too or automatically switch to using of the keyfile backend. I think that users won't know that they need to change it manually. If you tell me what is needed here, I can have a look at this.

I've also observed that writing the same value which is already in the database is not signalized when done on another host. This is probably due to the fact that the file is not changed if it already has the key with the value. But the "change" is signalized if you watch for changes locally. Maybe we could do this more consistent.

Other than that it works really well for me :). Thank you for working on this.

Tell me if you prefer to have these filled as new bugs.

Regards

Marek

(In reply to comment #43)
> It seems that client has to write a value locally (dconf write ...) before it
> starts to get notifications of changes (dconf watch ...) done on another host.

This surprises me.  For service-db backends, the first client should send an 'Init' message to the bus as soon as it does its first read.  It has to do that in order to see that the file is created (from the keyfile, by the service) so that it has something to read from.

If you are just doing 'dconf watch' then I can imagine that this would not happen, but I also don't consider it a bug in that case: how do you know the value changed if you don't know what the old value was (because you never read it)?

> It also doesn't actualize the value before the write hence the host sees old
> values.

Can you explain what you mean by this in more detail?

> This is related to the creation of GFileMonitor during initialization
> of DConfKeyfileWriter. Could we create the GFileMonitor during creation of the
> watch?

See comments above.

> Another thing is the monitoring of gvdb changes on NFS. It would be great if
> gvdb could use the GFileMonitor too or automatically switch to using of the
> keyfile backend. I think that users won't know that they need to change it
> manually. If you tell me what is needed here, I can have a look at this.

Ya... I'm thinking about doing something automatic here.

The reason I don't use GVDB on NFS is because it's pointless.  The use of GVDB is a performance thing (because you can mmap it and do hash lookups) and since we need the service doing explicit synchronisation (which involves iterating the whole thing), we already lose the performance edge from GVDB.

I don't think it's unreasonable for sysadmins on NFS-using systems to have to manually configure their dconf setup.

That said, I don't like the current syntax and I think maybe it should be automatic.

> I've also observed that writing the same value which is already in the database
> is not signalized when done on another host. This is probably due to the fact
> that the file is not changed if it already has the key with the value. But the
> "change" is signalized if you watch for changes locally. Maybe we could do this
> more consistent.

This is correct.  There is no way to tell about remote changes that didn't really change anything because there is no change to the file to be observed.

I guess we could avoid sending change notifications in the case of local non-changes too, but that would definitely be a completely separate issue.

(In reply to comment #44)
> (In reply to comment #43)
> > It seems that client has to write a value locally (dconf write ...) before it
> > starts to get notifications of changes (dconf watch ...) done on another host.
> 
> This surprises me.  For service-db backends, the first client should send an
> 'Init' message to the bus as soon as it does its first read.  It has to do that
> in order to see that the file is created (from the keyfile, by the service) so
> that it has something to read from.
> 
> If you are just doing 'dconf watch' then I can imagine that this would not
> happen, but I also don't consider it a bug in that case: how do you know the
> value changed if you don't know what the old value was (because you never read
> it)?

It works when testing this with full GNOME session. I used console + xinit + wmaker before.


> > Another thing is the monitoring of gvdb changes on NFS. It would be great if
> > gvdb could use the GFileMonitor too or automatically switch to using of the
> > keyfile backend. I think that users won't know that they need to change it
> > manually. If you tell me what is needed here, I can have a look at this.
> 
> Ya... I'm thinking about doing something automatic here.

Great!


> The reason I don't use GVDB on NFS is because it's pointless.  The use of GVDB
> is a performance thing (because you can mmap it and do hash lookups) and since
> we need the service doing explicit synchronisation (which involves iterating
> the whole thing), we already lose the performance edge from GVDB.

I see now.

see bug 703416 for further discussion on dconf-over-nfs