Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No change notification for o.fd.UDisks2.Filesystem.Size anymore #1008

Closed
mvollmer opened this issue Aug 23, 2022 · 8 comments · Fixed by #1042
Closed

No change notification for o.fd.UDisks2.Filesystem.Size anymore #1008

mvollmer opened this issue Aug 23, 2022 · 8 comments · Fixed by #1042
Labels
Milestone

Comments

@mvollmer
Copy link
Contributor

I think we lost the property change notifications with #949, no?

@tbzatek
Copy link
Member

tbzatek commented Aug 23, 2022

Ah right, it was changed to be on demand only. I knew why I wasn't convinced to push this change to stable...

Hmm, hmm, the problem was an endless uevent storm so we had to decouple that. Caused by certain filesystems that generate extra uevent once you retrieve the filesystem size. And we simply can't refresh the fs size as a result of a uvent, no matter how delayed the refresh would be. The change to the behaviour was a compromise between changing the API, deprecation and introducing a getter method.

If it's suitable for your use case, can you just use the built-in org.freedesktop.DBus.Properties.Get() method to trigger the refresh after you make any modification to the filesystem or on a first object retrieval? I suppose this value wouldn't change often.

@mvollmer
Copy link
Contributor Author

I think we really should fix dumpe2fs or stop using it. Here is a potential fix: tytso/e2fsprogs#122

To stop using it, libblockdev could use libext2fs directly instead of spawning dumpe2fs. That's probably a good idea in general. I'll have a stab at that.

@mvollmer
Copy link
Contributor Author

mvollmer commented Aug 24, 2022

Here is my first toy program, looks promising:

#include <stdio.h>
#include <ext2fs.h>

void
main (int argc, char **argv)
{
  errcode_t retval;
  ext2_filsys fs;

  retval = ext2fs_open (argv[1],
                        EXT2_FLAG_JOURNAL_DEV_OK | EXT2_FLAG_SOFTSUPP_FEATURES |
                        EXT2_FLAG_64BITS | EXT2_FLAG_THREADS | EXT2_FLAG_SUPER_ONLY |
                        EXT2_FLAG_IGNORE_CSUM_ERRORS,
                        0, // use_superblock
                        0, // use_blocksize
                        unix_io_manager,
                        &fs);

  if (retval) {
    printf ("OPEN %d\n", retval);
    exit (1);
  }

  printf ("%llu\n", ((unsigned long long) ext2fs_blocks_count(fs->super)) * EXT2_BLOCK_SIZE(fs->super));
}

@tbzatek
Copy link
Member

tbzatek commented Aug 24, 2022

I think we really should fix dumpe2fs or stop using it. Here is a potential fix: tytso/e2fsprogs#122

Nice!

One other major reason for making the property value retrieval on demand was to relieve I/O traffic. In practice, all other properties were retrieved from the (cached) udev database. In small systems this was not a big issue, the problem was daemon startup and first enumeration in case of large number of block devices.

We've had bugreports about the daemon not starting up in a reasonable time (30+ sec.) when autostarted from a first D-Bus call, resulting in the gvfs udisks2 volume monitor bailing out, falling back to local GIO unix monitoring. This happened on a system with rotational disks during higher load upon system startup.

@vojtechtrefny
Copy link
Member

vojtechtrefny commented Sep 1, 2022

Related: libblkid now has two new filesystem properties/tags: FSSIZE and FSLASTBLOCK (not yet released) which we potentially could use for getting filesystem size, it will be also likely available in udev (which just calls blkid) so we could get these "for free" without calling libblockdev.

@mvollmer
Copy link
Contributor Author

mvollmer commented Sep 2, 2022

libblkid now has two now filesystem two properties/tags: FSSIZE and util-linux/util-linux#1665

Nice!

@tbzatek
Copy link
Member

tbzatek commented Nov 10, 2022

I've tried to hack a fix for the property change notification - see #1042.
@mvollmer: do you have any chance to test this please? I have just been watching dbus-monitor output but the client D-Bus bindings may behave in various ways. Let's just hope that with this change we'd be conforming with the D-Bus specifications with relation to clients out there.

@tbzatek
Copy link
Member

tbzatek commented Feb 23, 2023

ping @mvollmer - any chance to test #1042 please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants