From: Jeff D. <jd...@ka...> - 2002-04-15 21:08:26
|
This looks pretty reasonable, but there's one thing I don't understand. You make ubd<digit> and ubd<letter> synonyms for each other, implying that the <digit> devices are the non-partitioned ones and the <letter> ones are partitioned. This means that there should be something avoiding calling register_disk on the <digit> devices, right? First, I don't see any code that does that, and second, hardly anything in the driver knows what the device name is anyway. ubd_open seems to be about it, so you'd have to pull the filename from the struct file to see whether it needs a partition check. Jeff |
From: McMechan, J. <McM...@na...> - 2002-04-15 22:39:14
|
No,..., I did that so that ubd[0..7] would still work and the device could also be called ubd[a..g] so that for a partitioned device ubda1 could be used instead of ubd01 which looks strange though it should work just fine. ubda can be used to refer to the whole disk just like ubd0 in the device files the /dev/ubd/disc[0..7]/disc and /dev/ubd/disc[0..7]/part[0-15] are what I use. You are correct the driver does not care and the only reason to use it in init/main.c is so that the root device can be slightly clearer. All the other root devices seem to use the driver.letter.number format and several other chunks like gendisk assume this also. I just do a partition check on all possible devices since all that happens if it does not work is it reports "unknown partition table" if it is a filesystem image, rather than a disk image, and it only registers the /dev/ubd/disc[0-7]/disc entry. I did leave the /dev/ubd/[0-7] entries so that old filesystems would still work. > ---------- > From: Jeff Dike > Sent: Monday, April 15, 2002 18:10 > To: McMechan, James > Cc: use...@li... > Subject: Re: [uml-devel] short version of partition patch > > This looks pretty reasonable, but there's one thing I don't understand. > > You make ubd<digit> and ubd<letter> synonyms for each other, implying that > the <digit> devices are the non-partitioned ones and the <letter> ones are > partitioned. > > This means that there should be something avoiding calling register_disk > on the <digit> devices, right? First, I don't see any code that does > that, > and second, hardly anything in the driver knows what the device name is > anyway. ubd_open seems to be about it, so you'd have to pull the filename > from the struct file to see whether it needs a partition check. > > Jeff > > |
From: Jeff D. <jd...@ka...> - 2002-04-15 23:38:38
|
McM...@na... said: > I just do a partition check on all possible devices since all that > happens if it does not work is it reports "unknown partition table" if > it is a filesystem image, rather than a disk image, and it only > registers the /dev/ubd/disc[0-7]/disc entry. OK, that's that I was worried about. I think I'll add some code to check the name and stick a 'has partitions' flag in the device structure. People will complain about that message if they ever see it. Jeff |
From: McMechan, J. <McM...@na...> - 2002-04-16 00:19:51
|
As you mentioned ~2 messages back that will be somewhat complex. The messages will only show up in the partition check report during boot or in the syslog during uml_mconsole config, Also the message is accurate. Were you thinking of a different syntax to indicate a partitionable device? perhaps ubd0p(artitioned) or ubd0f(ilesystem) or something similar:) I was trying to avoid that since the user can decide to change a devices configuration. For example having a old filesystem and converting to a disk image via fdisk or taking a disk image and converting it to a filesystem image via mke2fs. The name is only different for the root device anyway since only it needs to worry about partitions at the command line all the ubd images were treated the same by the ubd_config/setup/init sections are specified by device number. Example linux ubd0=part_root ubd1=file.img ubd2=disk.img ubd3=file2.img ubd4=disk2.img root=/dev/ubda1 does not lend itself to handling whether any particular ubd is a disk or filesystem image and why should the users have to worry about it. I might be even worse to have a option that is needed if the user can get it wrong. I think we could put in a message or something that explains what unknown partition table means or just convert the filesystems to disk images or even fake disk image/filesystem images > ---------- > From: Jeff Dike > Sent: Monday, April 15, 2002 20:40 > To: McMechan, James > Cc: use...@li... > Subject: Re: [uml-devel] short version of partition patch > > McM...@na... said: > > I just do a partition check on all possible devices since all that > > happens if it does not work is it reports "unknown partition table" if > > it is a filesystem image, rather than a disk image, and it only > > registers the /dev/ubd/disc[0-7]/disc entry. > > OK, that's that I was worried about. I think I'll add some code to check > the name and stick a 'has partitions' flag in the device structure. > > People will complain about that message if they ever see it. > > Jeff > > |
From: Jeff D. <jd...@ka...> - 2002-04-16 03:22:59
|
McM...@na... said: > Also the message is accurate. Were you thinking of a different syntax > to indicate a partitionable device? perhaps ubd0p(artitioned) or > ubd0f(ilesystem) or something similar:) I was trying to avoid that > since the user can decide to change a devices configuration. Yeah, you're right. A few comments - You didn't handle COW files properly. You did file_size on dev->file, which is the COW file, which may not yet be the full size of the backing file. The geometry calculation was wrong. It returned the number of bytes in a cylinder, not the number of sectors. This was my bug, but I'm surprised you didn't see it. I turned a 500M, 4 partition file into a 64G file by running mke2fs on one of those partitions. I'm getting devfs complaints from fdisk calling revalidate: devfs_mk_dir(ubd/disc1): using old entry in dir: a0accae0 "ubd" devfs_register(disc): could not append to parent, err: -17 devfs_mk_dir(ubd/disc1): using old entry in dir: a0accae0 "ubd" devfs_register(disc): could not append to parent, err: -17 The devfs_mk_dir one seems to be a warning, but the devfs_register one is an error. You don't get /dev/ubd/disc1/part* afterwards. Also, running a second time fdisk fails: usermode:~# fdisk /dev/ubd/disc1/disc Unable to read /dev/ubd/disc1/disc It's reading from /dev/ubd/disc1/disc and getting nothing: open("/dev/ubd/disc1/disc", O_RDWR) = 3 fstat(3, {st_mode=S_IFBLK|0600, st_rdev=makedev(98, 16), ...}) = 0 read(3, "", 512) = 0 Other than these problems, it seems to be behaving itself. Jeff |
From: McMechan, J. <McM...@na...> - 2002-04-18 09:09:24
Attachments:
part-update.20020418
|
> ---------- > From: Jeff Dike > Sent: Tuesday, April 16, 2002 00:25 > To: McMechan, James > Cc: use...@li... > Subject: Re: [uml-devel] short version of partition patch > > McM...@na... said: > > Also the message is accurate. Were you thinking of a different syntax > > to indicate a partitionable device? perhaps ubd0p(artitioned) or > > ubd0f(ilesystem) or something similar:) I was trying to avoid that > > since the user can decide to change a devices configuration. > > Yeah, you're right. > > A few comments - > > You didn't handle COW files properly. You did file_size on dev->file, > which > is the COW file, which may not yet be the full size of the backing file. I got confused, I thought dev->cow.file was the cow file and dev->file was the file so the attached patch fixes that. > The geometry calculation was wrong. It returned the number of bytes in a > cylinder, not the number of sectors. This was my bug, but I'm surprised > you didn't see it. I turned a 500M, 4 partition file into a 64G file by > running mke2fs on one of those partitions. I also added fixes for the two geometry calculations. And remembered to clean up ubd_remove. > I'm getting devfs complaints from fdisk calling revalidate: > > devfs_mk_dir(ubd/disc1): using old entry in dir: a0accae0 "ubd" > devfs_register(disc): could not append to parent, err: -17 > devfs_mk_dir(ubd/disc1): using old entry in dir: a0accae0 "ubd" > devfs_register(disc): could not append to parent, err: -17 These should be fixed. I was too aggressive in clearing the ubd_part array and accidentally killed the devfs internal handles > The devfs_mk_dir one seems to be a warning, but the devfs_register one is > an error. You don't get /dev/ubd/disc1/part* afterwards. > > Also, running a second time fdisk fails: > > usermode:~# fdisk /dev/ubd/disc1/disc > > Unable to read /dev/ubd/disc1/disc > > It's reading from /dev/ubd/disc1/disc and getting nothing: > > open("/dev/ubd/disc1/disc", O_RDWR) = 3 > fstat(3, {st_mode=S_IFBLK|0600, st_rdev=makedev(98, 16), ...}) = 0 > read(3, "", 512) = 0 > > Other than these problems, it seems to be behaving itself. These should be fixed also, I tested with both empty files and COW partitioned files, fdisking repeatedly worked and the devfs files changed. I am still missing the proper invocation to clear out the partition devfs files on ubd_remove but that is all I know of that does not work as I think it should. But then I thought the last version worked well when I sent it in. Booting, partitions, fdisk, device files ... > Jeff Patch against previous version of the partition patch as applied to 2.4.18-18um <<part-update.20020418>> if you would rather, I have instead a new version of the cleanup patch with ubd_remove etc. and a new version of the partition patch. |
From: Jeff D. <jd...@ka...> - 2002-04-20 03:05:37
|
McM...@na... said: > These should be fixed also, I tested with both empty files and COW > partitioned files, fdisking repeatedly worked and the devfs files > changed. I am still missing the proper invocation to clear out the > partition devfs files on ubd_remove but that is all I know of that > does not work as I think it should. Applied, this version is much better. Jeff |