crash when creating snapshot due to truncated sysfs name

[fajarnugraha]

I'm testing zpl branch, and got a crash when creating a snapshot. Console log shows something interesting:

------------[ cut here ]------------
WARNING: at fs/sysfs/dir.c:491 sysfs_add_one+0xdb/0xf3()
Hardware name: ProLiant BL460c G1
sysfs: cannot create duplicate filename '/block/vm!win2008r2ent-template!xvda@t'

... and further below

kobject_add_internal failed for vm!win2008r2ent-template!xvda@t with -EEXIST, don't try to register things with the same name in the same directory.

... then

------------[ cut here ]------------
kernel BUG at fs/sysfs/group.c:65!
invalid opcode: 0000 [#1] SMP 
last sysfs file: /sys/block/vm!win2008r2ent-template!xvda@t/removable

... and finally

---[ end trace 042bc5835f9d13d4 ]---
Kernel panic - not syncing: Fatal exception

The name is truncated. It's supposed to be :

• '/block/vm!win2008r2ent-template!xvda@template' (the one I created first), and
• '/block/vm!win2008r2ent-template!xvda@test' (the one I just created, which cause the crash)

Perhaps we should change sysfs filename to use something like sequential numbers (zvol-1, zvol-2, etc) and keep symlinks/mappings somewhere? It would be similar to LVM, which simply use dm-X for sysfs, but the mapping can be traced by looking at minor number in /dev/mapper directory.

[fajarnugraha]

Adding some info to the previous post, what happened was:

• I already have a snapshot called vm/win2008r2ent-template/xvda@template
• I created a new snapshot, vm/win2008r2ent-template/xvda@test
• this cause kernel panic, with the above output

[behlendorf]

So based on your comments it looks like it's truncating the sysfs name to the first 32 characters. We could limit a snapshot name to 32 characters to avoid this, or maybe just add a check to the ZFS code to detect if your using the first 32 characters as an existing snapshot and warn you (it's better than crashing).

[fajarnugraha]

IMHO a warning (and not creating the sysfs file) is better.

We should not put a zfsonlinux-specific limit to the name (during creation) since solaris supports long dataset (including snapshot) names just fine (tested with "rpool/some-really-long-dataset-name-01234567890-01234567890-01234567890"). That way moving pools back and forth between solaris-linux will (mostly work), with the exception that you can't access zvols with long names (but no data loss or crash either)

I still think it's better to use short names with incremental number for sysfs though.

[behlendorf]

The trouble with using an alternate name for the sysfs entry is that the sysfs entry is what udev uses to create the /dev/ entry. In other words if we change the sysfs name the /dev/ name changes too. That is unless we do something extra clever and provide custom udev rules for zvols to do the name mapping your proposing. That could be done, and off the top of my head the pros and cons I can see are:

pros

1. Support of to 255 byte long snapshot names without any real character restrictions.
2. Could adopt the Solaris /dev/zvol/ naming convention for all supported kernels.

cons

1. Have to disable the existing block device udev rules for zvol devices.
2. Have to provide our own udev rule for zvol devices
3. Supporting this could be difficult since each distribution has their own udev quirks. Usually these relate the version of udev used by the distribution and the particular default rule set they have settled on.

[fajarnugraha]

Here's an idea on how to use alternate sysfs entry:

1. use sysfs entries like zvol!devices!zvol-1 , where the last number is auto-increment. This should create /dev/zvol/devices/zvol-1, using existing block device udev rules. Snapshots also gets its own auto-increment number.
2. create additional udev rule similar to persistent disk links: /dev/disk/{by-id,by-uuid,by-label,by-path}, but in this case
   2.a) we limit it to KERNEL=="zvol*"
   2.b) create the links in /dev/zvol/dsk/
   For this to work we need something for zvol with similar functionality as /lib/udev/vol_id to map device nodes to dataset names (do we have this mapping somewhere? By looking at device minor number, maybe?)
3. create a symlin /dev/zvol/rdsk, which points to dsk

This way at least we don't have to worry about cons #1, since we're simply adding rules. cons #2 should be a one-time work. This leaves cons #3, but since most distros already support /dev/disk/{by-id,by-uuid,by-label,by-path} link scheme, and we're going to use similar mechanism, it should be possible.

What do you think?

[behlendorf]

OK, I'm sold. I think your right, this is the only way to cleanly solve the issue long term. However, if we're going to do this I think we should do it completely the Linux way.

1. Register the zvol block devices with the names zd[0-N]. This will cause similarly named sysfs entries and top level entries in /dev/, just like md[0-N].
2. Add additional udev rules just like the persistent disks links.
3. This requires adding a udev helper. The zfs user package already does this for the zpool mapping we'll just need another one. The mapping can be accomplished by adding additional sysfs entires which contain the dataset name to the zv[0-N] sysfs directory. This is exactly how standard block devices are handled. We can additionally add any other information which might be useful to this directory so facilitate crafting custom udev rules.

[fajarnugraha]

For #1, any hints on which file/function needs to be changed?

For #2, should be easy when all necessary requirements are in place

For #3, I thought we could use "DEVNAME" from /sys/block/*/uevent, but that would depend on what name we use to register the block device, isn't it? any hints on which file/function needs to be changed to add another sysfs attribute?

[behlendorf]

The bulk/all of the the changes needed for changing the device name will be in module/zfs/zvol.c. The function __zvol_create_minor() is responsible for allocating a new zvol_state_t structure for the device via zvol_alloc(), it is and then registered as a new block device with the kernel via add_disk(). Once add_disk() returns the device is live so some care is needed. It will also contain all the standard sysfs entries related to a block device. We of course want to add our own additional entry containing the real name of udev to consume. There's a nice article at lwn which shows how to basically go about this, http://lwn.net/Articles/54651/.

As for the udev helpers there one existing helper in cmd/zpool_id/zpool_id and rules which leverage it in etc/udev/rules.d/60-zpool.rules. I would suggest we add a cmd/zvol_id/zvol_id and an etc/dev/rules.d/60-zvol.rules.

[fajarnugraha]

The easy part: zvol_id might not be necessary for udev if we can create a custom sysfs entry, and the proposed /etc/dev/rules.d/60-zvol.rules can just use that.

Changing the device name is also easy enough

diff -Naru behlendorf-zfs-037849f/module/zfs/zvol.c behlendorf-zfs-037849f.new/module/zfs/zvol.c
--- behlendorf-zfs-037849f/module/zfs/zvol.c    2011-02-18 05:23:48.000000000 +0700
+++ behlendorf-zfs-037849f.new/module/zfs/zvol.c        2011-02-21 13:28:34.980400341 +0700
@@ -1072,7 +1072,7 @@
        zv->zv_disk->fops = &zvol_ops;
        zv->zv_disk->private_data = zv;
        zv->zv_disk->queue = zv->zv_queue;
-       snprintf(zv->zv_disk->disk_name, DISK_NAME_LEN, "%s", name);
+       snprintf(zv->zv_disk->disk_name, DISK_NAME_LEN, "zd%d", zv->zv_disk->first_minor);
 
        return zv;

We probably should replace "zd" with a constant defined somewhere, but it's enough for test purposes. This results in dev nodes created as /dev/zv*. There's a message about

/dev/[dataset_name] may not be immediately available

due to libzfs_dataset.c doing

    (void) snprintf(path, sizeof (path), "%s/%s", ZVOL_DIR, dataset);
    error = zpool_label_disk_wait(path, 10000);

but I think that can be ignored for now. Since zpool_label_disk_wait simply uses stat64(), creating a compatibility symlink using udev in the old /dev/vm/dataset_name format should solve this error message.

The hard part, is how to create additional sysfs entry. The link you sent is very good (albeit still being high level). So I started with simply putting the example from samples/kobject/kobject-example.c in Linux tree (online archive for 2.6.32.27: http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.32.y.git;a=blob_plain;f=samples/kobject/kobject-example.c;hb=a386bf75dea7208041b5d4b8d44726876e28ee4b), and putting it on zvol.c. However linking process failed with

make[3]: Entering directory `/usr/src/kernels/2.6.32.27-1.pv_ops.el5.fan-xen-x86_64'
  CC [M]  /data/zfs/zpl/behlendorf-zfs-037849f/module/zfs/../../module/zfs/zvol.o
  LD [M]  /data/zfs/zpl/behlendorf-zfs-037849f/module/zfs/zfs.o
  Building modules, stage 2.
  MODPOST 6 modules
FATAL: modpost: GPL-incompatible module zfs.ko uses GPL-only symbol 'sysfs_create_group'
make[4]: *** [__modpost] Error 1
make[3]: *** [modules] Error 2

Same thing with sysfs_create_file. Any idea how to solve this?

[fajarnugraha]

Looking from another angle, the information needed to create correct symlinks is the mapping between dataset name and minor number. This information is available in kernel space in zv->zv_dev and zv->zv_name, where the list of zvs are in zvool_state_list. While sysfs provide one way, is there another (possibly easier) way to acess these information from userland? By adding them to the list of attributes returned by "zfs get", maybe?

I'm beginning to think the stupid-yet-easy way is to force the module to create a mapping manually in /etc/zfs using kernel's syscall open-write-close

[behlendorf]

Great start on this!

Your right, we might not need zvol_id if we can get the attribute setup properly. If we can't we might need to use it to do a slightly more complicated lookup to get the name.

Also we shouldn't worry about zpool_label_disk_wait() yet, it's just there to cause the command link tool to wait until the link appears. This just makes scripting using the tools easier.

As for the GPL-only symbol warning that's unfortunate. You've done everything right here but the kernel developers have decided that only pure GPL kernel module should be able to use that function. As I'm sure you know the zfs code is CDDL which means were limited to a smaller set of available functions. I've had to fight with this sort of problem a lot and it usually means finding a less clean work around. In some cases that means reimplementing the missing code we need, or in this case creating the links in a different way as you suggested.

I don't link the idea of having the ZFS code itself create the links, that sort of thing while possible is seriously frowned upon. I think it would be better to go with your first idea of just making the name available to userspace via some other method. There are quite a few options available for that.

1. We could extend 'zfs get' to return the information.
2. We could create a /proc/zvols entry with shows all the zd[0-N] -> name mappings.
3. We could register an ioctl() handler for the zd[0-N] device which returns its name.

All of these would require the addition of the zvol_id helper, which is a shame because registering the attribute was such a clean fix. Of the 3 above I think #3 is the simplest mainly because we don't need to do any parsing. Just add a new custom ioctl for zvol block devices. This can be done in zfs_ioctl(), currently just BLKFLSBUF is supported which is used to sync the device. We can add a BLKZNAME which just returns the name to user space. Then your zvol_id helper can run this ioctl on the device and use it to create the link. Sound good to you?

[devsk]

I think I ran into this very bug while trying to import a test pool exported from zfs-fuse. It has names which likely make the path go beyond 32 chars.

60935.423741] WARNING: at fs/sysfs/dir.c:451 sysfs_add_one+0x8c/0xa1()
[60935.423744] Hardware name: Latitude E6510
[60935.423747] sysfs: cannot create duplicate filename '/devices/virtual/block/hitachi!zfs-backup!opensolaris-'
[60935.423750] Modules linked in: vboxnetadp vboxnetflt vboxdrv xfrm6_mode_tunnel ecb ipv6 snd_seq_oss snd_seq_midi_event snd_seq
 snd_seq_device snd_pcm_oss snd_mixer_oss tun snd_intel8x0 snd_ac97_codec ac97_bus acpi_cpufreq mperf cifs fuse crc32c_intel core
temp i915 snd_hda_codec_hdmi drm_kms_helper snd_hda_codec_idt drm i2c_algo_bit cfbcopyarea snd_hda_intel snd_hda_codec uvcvideo snd_pcm wl(P) snd_timer sdhci_pci videodev v4l1_compat sdhci v4l2_compat_ioctl32 snd intel_agp e1000e mmc_core intel_gtt snd_page_alloc cfbimgblt cfbfillrect agpgart evdev video backlight zfs(P) zcommon(P) zunicode(P) znvpair(P) zavl(P) splat spl tg3 libphy e1000 xfs exportfs nfs auth_rpcgss lockd sunrpc jfs reiserfs ohci_hcd uhci_hcd usb_storage ehci_hcd sata_sil24 sata_sil scsi_wait_scan pata_jmicron pata_amd pata_mpiix [last unloaded: tunnel6]
[60935.423836] Pid: 11016, comm: zpool Tainted: P            2.6.37.1 #6
[60935.423839] Call Trace:
[60935.423848]  [] ? warn_slowpath_common+0x78/0x8c
[60935.423853]  [] ? warn_slowpath_fmt+0x45/0x4a
[60935.423859]  [] ? sysfs_add_one+0x8c/0xa1
[60935.423863]  [] ? create_dir+0x67/0x9f
[60935.423868]  [] ? sysfs_create_dir+0x89/0x9f

[fajarnugraha]

Please review fajarnugraha@fe69ba5

Tested on my dev server and works fine so far. We should work more on cmd/zvol_id though, since currently you need to run "make && make install" manually (I don't know how to correctly integrate it with the rest of the code yet)

Edit: corrected commit link.

[devsk]

What happens when udev is not around, e.g. during initramfs part of the boot. Its not a big deal because pool name is small enough (we can ask folks to use rpool/root for the rootfs) and small snapshot names will get us by but we need to fix this eventually.

[fajarnugraha]

The proposed change here is zvol-only (which creates block device entries), and should not affect zfs filesystems directly (which does not need a block device). It might be a problem if one uses e.g. ext4-formatted zvol for root, but is this really a common case?

If you use zfs for root, you might need to create /dev/zfs manually (https://groups.google.com/a/zfsonlinux.org/group/zfs-devel/browse_thread/thread/2c7f00204858c7c0?pli=1).

That being said, the current code chooses minor numbers (which I also use as zd* numbers) in a somewhat non-persistent manner (e.g. creating/deleting a dataset can cause minor number change for another dataset after reboots). I wonder if we should use dataset ID instead (which should be fixed for its life time, even after "zfs rename")

[devsk]

rpool/root is marked legacy and is manually mounted during boot, so /dev/zfs is probably not needed. Otherwise, the boot wouldn't have worked for me at all.

So, you are saying the Oops pasted by me above, is a different issue altogether?

[fajarnugraha]

This one?

60935.423747] sysfs: cannot create duplicate filename '/devices/virtual/block/hitachi!zfs-backup!opensolaris-'

It should be the same thing as mine, caused by two or more zvol (or zvol snapshot) names truncated and ended up having the same sysfs name. It shouldn't show up at all if:

• you don't use zvol (in which case no sysfs file will be created), or
• don't create long entries for sysfs (which is what I proposed, using short device names and put the long names using symlinks)

[devsk]

device name entries are created by ZFS, so I did not do anything wrong...:-)

There is no zvol involved here. The above happened when I tried to import a backup pool exported from zfs-fuse. It has pretty long names for the filesystems. But then we don't have many bytes left after "/devices/virtual/block/", do we?

which is what I proposed, using short device names and put the long
names using symlinks

Is that a part of another issue or already part of your patch in this issue?

[behlendorf]

Fajarnugraha, great job on this, I think you've got exactly the right approach. I've made a first pass reviewing your change and commented in the commit. Can you please address my concerns and I'll be happy to review the patch again. Thanks again for taking on this bug. fajarnugraha@fe69ba5#commitcomment-279738

Devsk, these changes are only related to zvol devices. Normal zfs datasets don't have any need to create devices in sysfs, only zvol block devices. From your comment however it looks like your system does have at least one zvol (or zvol snapshot) named hitachi/zfs-backup/opensolaris-. While the zfs-fuse port couldn't have created this, could this originally have been created on a Solaris system?

[devsk]

Ok. The name was truncated. But my guess is that its the swap zvol from the opensolaris install, the path for which is hitachi/zfs-backup/opensolaris-backup/rpool/swap.

[fajarnugraha]

I was so sure I posted an update on this issue, but now it's missing.

Oh well,

Revised proposed fix is on https://github.com/fajarnugraha/zfs/commits/master , the revision is split into three additional commits

[behlendorf]

Fajar this looks nice! I squashed your 4 commits in to a single commit to make it easier for me to review. I only found two small issues which I fixed and some other trivial cleanup. I've pushed my squashed commit to the zvol-name branch in my repo and have posted my comments with the commit. If after you review my trivial modifications your still happy with it I'll pull it in to master.

https://github.com/behlendorf/zfs/tree/zvol-name

Thanks for doing all the work on this! It works great.

[fajarnugraha]

Tested your final version (behlendorf@4e3ad44) by creating long zvol name, works great. Please pull it in to master.

[behlendorf]

Closing issue, Fajar's patch has been merged in to master, 4c0d8e5