Saggi Mizrahi has posted comments on this change. Change subject: mounts: Optimize mount loop device resolution ......................................................................
Patch Set 6: (3 comments) http://gerrit.ovirt.org/#/c/28586/6//COMMIT_MSG Commit Message: Line 9: Existing code was re-reading mtab for each /proc/mounts entry. Line 10: Added lookup dictionary that translates /dev/loop spec to actual file Line 11: spec. Line 12: Line 13: Lookup is reinitialized each time mtab is changed. > Please show profiling results of an operation that use lot of mount calls. The advantages can be trivially proven. The algorithm Line 14: Line 15: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1112779 Line 16: Change-Id: I54f11786b45782cedd994d52e1e506292132fa47 http://gerrit.ovirt.org/#/c/28586/6/vdsm/storage/mount.py File vdsm/storage/mount.py: Line 105: Loop devices appear as the loop device under /proc/mount instead of the Line 106: backing file. As the mount command does the resolution so must we. Line 107: """ Line 108: if not path.startswith("/"): Line 109: return path > This is a case of "could you also" style of comment. I don't want to have that assumption. $ dd if=/dev/urandom of=test.img $ losetup -f test.img $ mv /dev/loop0 /dev/frutyloop $ ls /dev/frutyloop /dev/frutyloop This is the same reason I don't use devname as in sysfs it is still located in /sys/block/loop0/ since the kernel is unaware of this change. This is why the only official handle for a device is it's major:minor. Line 110: Line 111: try: Line 112: st = os.stat(path) Line 113: except: Line 123: 'loop') Line 124: if os.path.exists(loopdir): Line 125: with open(loopdir + "/backing_file", "r") as f: Line 126: # Remove trailing newline Line 127: return f.read()[:-1] > There is no reason to check if the directory exists, just try to open and r I don't want to use strip() or replace() as they might remove too much. We have seen devices that are named with just whitespace. I want to remove the last char. That is what I do. I comment on what is that last char since I understand it might not be apparent why I do that. I could make a function. RemoveTrailingNewline() but that seems like an overkill. Line 128: Line 129: # Old kernels might not have the sysfs entry, this is a bit slower and does Line 130: # not work on hosts that do support the above method. Line 131: -- To view, visit http://gerrit.ovirt.org/28586 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I54f11786b45782cedd994d52e1e506292132fa47 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dima Kuznetsov <dkuzn...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Dima Kuznetsov <dkuzn...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
