[PATCH] pktcdvd & udf bugfixes

[PATCH] pktcdvd & udf bugfixes

[Phillip Susi]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

Attached is a patch to fix a few bugs in the pktcdvd driver and udf 
filesystem.  Ben Collins said I should post it to the list and cc Jens 
Axboe as he works on this area.  The patch is rather short, but fixes 
the following bugs:

1) The pktcdvd driver was using an 8 bit field to store the packet 
length obtained from the disc track info.  This causes it to overflow 
packet length values of 128 sectors or more.  I changed the field to 32 
bits to fix this.

2) The pktcdvd driver defaulted to it's maximum allowed packet length 
when it detected a 0 in the track info field.  I changed this to fail 
the operation and refuse to access the media.  This seems more sane than 
attempting to access it with a value that almost certainly will not work.

3) The pktcdvd driver uses a compile time macro constant to define the 
maximum supported packet length.  I changed this from 32 sectors to 128 
sectors because that allows over 100 MB of additional usable space on a 
700 MB cdrw, and increases throughput.

4) The UDF filesystem refused to update the file's uid and gid on the 
disk if the in memory inode's id matched the values in the uid= and gid= 
mount options.  This was causing the owner to change from the desktop 
user to root when the volume was ejected and remounted.  I changed this 
so that if the inode's id matches the mount option, it writes a -1 to 
disk, because when the filesystem reads a -1 from disk, it uses the 
mount option for the in memory inode.  This allows you to use the 
uid/gid mount options in the way you would expect.

At some point I hope to find the time to refactor pktcdvd to properly 
allocate buffers of the length specified on the disc rather than the 
compile time maximum, but that will be a larger change and require more 
testing.


[-- Attachment #2: pktcdvd_len_fix+udf_uid_fix.diff --]
[-- Type: text/x-patch, Size: 2112 bytes --]

diff -r -u linux-source-2.6.15/drivers/block/pktcdvd.c linux-source-2.6.15.new/drivers/block/pktcdvd.c
--- linux-source-2.6.15/drivers/block/pktcdvd.c	2005-11-14 12:44:28.000000000 -0500
+++ linux-source-2.6.15.new/drivers/block/pktcdvd.c	2006-01-03 20:00:01.000000000 -0500
@@ -1639,7 +1639,7 @@
 	pd->settings.size = be32_to_cpu(ti.fixed_packet_size) << 2;
 	if (pd->settings.size == 0) {
 		printk("pktcdvd: detected zero packet size!\n");
-		pd->settings.size = 128;
+		return -ENXIO;
 	}
 	if (pd->settings.size > PACKET_MAX_SECTORS) {
 		printk("pktcdvd: packet size is too big\n");
Only in linux-source-2.6.15.new/drivers/block: pktcdvd.c~
diff -r -u linux-source-2.6.15/fs/udf/inode.c linux-source-2.6.15.new/fs/udf/inode.c
--- linux-source-2.6.15/fs/udf/inode.c	2005-11-02 11:29:10.000000000 -0500
+++ linux-source-2.6.15.new/fs/udf/inode.c	2006-01-03 19:02:23.000000000 -0500
@@ -1342,9 +1342,11 @@
 
 	if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
 		fe->uid = cpu_to_le32(inode->i_uid);
+	else fe->uid = cpu_to_le32(-1);
 
 	if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
 		fe->gid = cpu_to_le32(inode->i_gid);
+	else fe->gid = cpu_to_le32(-1);
 
 	udfperms =	((inode->i_mode & S_IRWXO)     ) |
 			((inode->i_mode & S_IRWXG) << 2) |
diff -r -u linux-source-2.6.15/include/linux/pktcdvd.h linux-source-2.6.15.new/include/linux/pktcdvd.h
--- linux-source-2.6.15/include/linux/pktcdvd.h	2005-11-02 11:29:13.000000000 -0500
+++ linux-source-2.6.15.new/include/linux/pktcdvd.h	2006-01-02 21:48:41.000000000 -0500
@@ -114,7 +114,7 @@
 
 struct packet_settings
 {
-	__u8			size;		/* packet size in (512 byte) sectors */
+	__u32			size;		/* packet size in (512 byte) sectors */
 	__u8			fp;		/* fixed packets */
 	__u8			link_loss;	/* the rest is specified
 						 * as per Mt Fuji */
@@ -169,7 +169,7 @@
 #if (PAGE_SIZE % CD_FRAMESIZE) != 0
 #error "PAGE_SIZE must be a multiple of CD_FRAMESIZE"
 #endif
-#define PACKET_MAX_SIZE		32
+#define PACKET_MAX_SIZE		128
 #define PAGES_PER_PACKET	(PACKET_MAX_SIZE * CD_FRAMESIZE / PAGE_SIZE)
 #define PACKET_MAX_SECTORS	(PACKET_MAX_SIZE * CD_FRAMESIZE >> 9)
 

Re: [PATCH] pktcdvd & udf bugfixes

[Pekka Enberg]

Hi,

On 1/12/06, Phillip Susi <psusi@cfl.rr.com> wrote:
> Attached is a patch to fix a few bugs in the pktcdvd driver and udf
> filesystem.  Ben Collins said I should post it to the list and cc Jens
> Axboe as he works on this area.  The patch is rather short, but fixes
> the following bugs:

Please read Documentation/SubmittingPatches. The pktcdvd and UDF fixes
should be separate patches and you should cc the appropriate
maintainers which you can find in the MAINTAINERS file. Thanks.

                                         Pekka

Re: [PATCH] pktcdvd & udf bugfixes

[Peter Osterlund]

Phillip Susi <psusi@cfl.rr.com> writes:

> Attached is a patch to fix a few bugs in the pktcdvd driver and udf
> filesystem.  Ben Collins said I should post it to the list and cc Jens
> Axboe as he works on this area.  The patch is rather short, but fixes
> the following bugs:
> 
> 1) The pktcdvd driver was using an 8 bit field to store the packet
> length obtained from the disc track info.  This causes it to overflow
> packet length values of 128 sectors or more.  I changed the field to
> 32 bits to fix this.

The variable is unsigned, so it supports values up to 255, ie no need
to change it.

> 2) The pktcdvd driver defaulted to it's maximum allowed packet length
> when it detected a 0 in the track info field.  I changed this to fail
> the operation and refuse to access the media.  This seems more sane
> than attempting to access it with a value that almost certainly will
> not work.

That code is very old, I think Jens wrote it. I assume it wasn't just
for fun, but to be able to support drives with slightly
broken/non-standard firmware.

> 3) The pktcdvd driver uses a compile time macro constant to define the
> maximum supported packet length.  I changed this from 32 sectors to
> 128 sectors because that allows over 100 MB of additional usable space
> on a 700 MB cdrw, and increases throughput.

The current limit is 32 disc blocks, ie 64KB or 128 "linux sectors".

How do you make the packet size larger for a CDRW disc? Just changing
the constant is not going to help unless you can also format a disc
with larger packets.

> At some point I hope to find the time to refactor pktcdvd to properly
> allocate buffers of the length specified on the disc rather than the
> compile time maximum, but that will be a larger change and require
> more testing.

Might be a good idea. On DVD discs the block size is only 32KB, so
half of the allocated memory is unused.

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] pktcdvd & udf bugfixes

[Phillip Susi]

Peter Osterlund wrote:
> 
> 
> The variable is unsigned, so it supports values up to 255, ie no need
> to change it.
> 
> 

The packet length is read and left shifted by two before being stored in 
that variable to convert it from 2048 byte sectors to 512 byte sectors, 
thus a value of 128 overflowed and became zero.  be32_to_cpu returns a 
32 bit value so I figured it should be stored in a 32 bit variable, not 
an 8 bit one.

> 
> That code is very old, I think Jens wrote it. I assume it wasn't just
> for fun, but to be able to support drives with slightly
> broken/non-standard firmware.
> 
> 

Yes, I hope he can let me know if that is the case, but right now I do 
not see how that can be.  As far as I know, that value is put there by 
the utility used to format the track, and _should_ be the correct 
length, never 0.  In any case, if it is zero, then assuming the maximum 
supported size would cause errors if the actual packet size is larger 
than the maximum that the driver supports.

> 
> The current limit is 32 disc blocks, ie 64KB or 128 "linux sectors".
> 
> How do you make the packet size larger for a CDRW disc? Just changing
> the constant is not going to help unless you can also format a disc
> with larger packets.
> 
> 

I also have several patches to the udftools package, one of which 
documents ( in the man page ) the previously undocumented -z packet_size 
parameter to cdrwtool, and fixes the code so that it actually works with 
values other than 32.

The upstream project for udftools on sourceforge appears to be dead.  I 
have sent email to the two original authors and had no reply, and the 
CVS repository has not been touched in over a year, and the mailing list 
is dead.  I am not sure what I should do about that, but in the mean 
time, I am maintaining ubuntu specific patches and have been speaking to 
the debian package maintainer about merging them there as well.


> Might be a good idea. On DVD discs the block size is only 32KB, so
> half of the allocated memory is unused.
> 

Why is it only 32 KB on a dvd?  What utility was used to format it like 
that?  My cd/dvd-rw drive blew out the cd laser the other day, and I got 
the replacement last night with some dvd+rw media, so I guess I will 
start playing with that soon.  From what I have read so far, dvd+rw 
media does not require pktcdvd to write to it, but its use can improve 
throughput.

Re: [PATCH] pktcdvd & udf bugfixes

[Peter Osterlund]

Phillip Susi <psusi@cfl.rr.com> writes:

> Peter Osterlund wrote:
> > The variable is unsigned, so it supports values up to 255, ie no need
> > to change it.
> 
> The packet length is read and left shifted by two before being stored
> in that variable to convert it from 2048 byte sectors to 512 byte
> sectors, thus a value of 128 overflowed and became zero.  be32_to_cpu
> returns a 32 bit value so I figured it should be stored in a 32 bit
> variable, not an 8 bit one.

OK, it makes sense if you can make packets larger than 64KB. I didn't
know you could.

> > The current limit is 32 disc blocks, ie 64KB or 128 "linux sectors".
> > How do you make the packet size larger for a CDRW disc? Just changing
> > the constant is not going to help unless you can also format a disc
> > with larger packets.
> 
> I also have several patches to the udftools package, one of which
> documents ( in the man page ) the previously undocumented -z
> packet_size parameter to cdrwtool, and fixes the code so that it
> actually works with values other than 32.

OK, so it appears you can make packets bigger than 64KB. Can I please
have those patches so I can test this myself.

> The upstream project for udftools on sourceforge appears to be dead.
> I have sent email to the two original authors and had no reply, and
> the CVS repository has not been touched in over a year, and the
> mailing list is dead.  I am not sure what I should do about that, but
> in the mean time, I am maintaining ubuntu specific patches and have
> been speaking to the debian package maintainer about merging them
> there as well.

I'm not sure either what to do if the project admin (Ben Fennema) is
no longer online so he can transfer maintenance to someone else.

> > Might be a good idea. On DVD discs the block size is only 32KB, so
> > half of the allocated memory is unused.
> 
> Why is it only 32 KB on a dvd?  What utility was used to format it
> like that? 

According to

        http://fy.chalmers.se/~appro/linux/DVD+RW/

"... native DVD ECC blocksize, which is 32KB"

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] pktcdvd & udf bugfixes

[Phillip Susi]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

Peter Osterlund wrote:
  > OK, so it appears you can make packets bigger than 64KB. Can I please
> have those patches so I can test this myself.
> 
> 

Sure, patches attached.

patch-6 is the one you are interested in for the different sizes, but I 
find the other two to be handy as well. patch-7 goes with the previous 
kernel udf patch I originally posted to fix permissions problems, and 
patch-8 adds a single parameter form to pktsetup for it to auto assign 
the new pktcdvd device and print it's minor number, rather than create a 
named devnode.  This is to facilitate hald auto configuring cdrw drives 
for packet writing, and let udev handle the creation of the devnode.

Don't forget to apply the kernel patch so you can actually write to the 
disc when you format it with >64 KB packets.


[-- Attachment #2: patch-6-nondefault-packet-size.diff --]
[-- Type: text/x-patch, Size: 4100 bytes --]

This patch fixes several assumptions in cdrwtool about the
packet size being 32 sectors which caused breakage when
the -z option was used to change the packet size.  Also
documented the -z option in the cdrwtool man page.  

 - Phillip Susi <psusi@cfl.rr.com>

diff -ru udftools-1.0.0b3.orig/cdrwtool/cdrwtool.h udftools-1.0.0b3/cdrwtool/cdrwtool.h
--- udftools-1.0.0b3.orig/cdrwtool/cdrwtool.h	2002-11-26 02:18:50.000000000 -0500
+++ udftools-1.0.0b3/cdrwtool/cdrwtool.h	2006-01-03 21:02:25.000000000 -0500
@@ -89,6 +89,7 @@
 	unsigned int	close_track;
 	unsigned int	close_session;
 	struct udf_disc	udf_disc;
+	int fd;
 };
 
 #ifndef be16_to_cpu
diff -ru udftools-1.0.0b3.orig/cdrwtool/defaults.c udftools-1.0.0b3/cdrwtool/defaults.c
diff -ru udftools-1.0.0b3.orig/cdrwtool/main.c udftools-1.0.0b3/cdrwtool/main.c
--- udftools-1.0.0b3.orig/cdrwtool/main.c	2004-02-22 22:33:11.000000000 -0500
+++ udftools-1.0.0b3/cdrwtool/main.c	2006-01-03 21:18:47.000000000 -0500
@@ -41,14 +41,15 @@
 {
 	static char *buffer = NULL;
 	static int bufferlen = 0, lastpacket = -1;
-	int fd = *(int *)disc->write_data;
+	struct cdrw_disc *cdisc = (struct cdrw_disc *)disc->write_data;
+	int fd = cdisc->fd;
 	int offset, packet;
 	struct udf_desc *desc;
 	struct udf_data *data;
 
 	if (buffer == NULL)
 	{
-		bufferlen = disc->blocksize * 32;
+		bufferlen = disc->blocksize * cdisc->packet_size;
 		buffer = calloc(bufferlen, 1);
 	}
 
@@ -56,7 +57,7 @@
 	{
 		if (lastpacket != -1)
 		{
-			if (write_blocks(fd, buffer, lastpacket * 32, 32) < 0)
+			if (write_blocks(fd, buffer, lastpacket * cdisc->packet_size, cdisc->packet_size) < 0)
 				return -1;
 		}
 	}
@@ -65,21 +66,21 @@
 		desc = ext->head;
 		while (desc != NULL)
 		{
-			packet = (uint64_t)(ext->start + desc->offset) / 32;
+			packet = (uint64_t)(ext->start + desc->offset) / cdisc->packet_size;
 			if (lastpacket != -1 && packet != lastpacket)
 			{
-				if (write_blocks(fd, buffer, lastpacket * 32, 32) < 0)
+				if (write_blocks(fd, buffer, lastpacket * cdisc->packet_size, cdisc->packet_size) < 0)
 					return -1;
 				memset(buffer, 0x00, bufferlen);
 			}
 			data = desc->data;
-			offset = ((uint64_t)(ext->start + desc->offset) - (packet * 32)) << disc->blocksize_bits;
+			offset = ((uint64_t)(ext->start + desc->offset) - (packet * cdisc->packet_size)) << disc->blocksize_bits;
 			while (data != NULL)
 			{
 				if (data->length + offset > bufferlen)
 				{
 					memcpy(buffer + offset, data->buffer, bufferlen - offset);
-					if (write_blocks(fd, buffer, packet * 32, 32) < 0)
+					if (write_blocks(fd, buffer, packet * cdisc->packet_size, cdisc->packet_size) < 0)
 						return -1;
 					memset(buffer, 0x00, bufferlen);
 					lastpacket = ++ packet;
@@ -94,9 +95,9 @@
 					offset += data->length;
 				}
 
-				if (offset == disc->blocksize * 32)
+				if (offset == disc->blocksize * cdisc->packet_size)
 				{
-					if (write_blocks(fd, buffer, packet * 32, 32) < 0)
+					if (write_blocks(fd, buffer, packet * cdisc->packet_size, cdisc->packet_size) < 0)
 						return -1;
 					memset(buffer, 0x00, bufferlen);
 					lastpacket = -1;
@@ -248,7 +249,8 @@
 		return fd;
 	}
 	disc.udf_disc.write = write_func;
-	disc.udf_disc.write_data = &fd;
+	disc.fd = fd;
+	disc.udf_disc.write_data = &disc;
 
 	if ((ret = cdrom_open_check(fd)))
 	{
diff -ru udftools-1.0.0b3.orig/debian/changelog udftools-1.0.0b3/debian/changelog
diff -ru udftools-1.0.0b3.orig/doc/cdrwtool.1 udftools-1.0.0b3/doc/cdrwtool.1
--- udftools-1.0.0b3.orig/doc/cdrwtool.1	2002-11-26 02:18:51.000000000 -0500
+++ udftools-1.0.0b3/doc/cdrwtool.1	2006-01-03 21:26:00.000000000 -0500
@@ -127,6 +127,10 @@
 variable and fixed packet sizes respectively.
 .IP "\fB\-o \fIoffset\fP"
 Set write offset.
+.IP "\fB\-z \fIsize\fP"
+Set packet length in sectors ( default = 32 ).  Longer packets allow for
+more usable space on the disc, but may slow down access to smaller files.  
+Switching from 32 to 128 on a 700 MB cdrw gives about 100 MB more usable space.
 
 .SH AUTHORS
 .nf
diff -ru udftools-1.0.0b3.orig/mkudffs/defaults.c udftools-1.0.0b3/mkudffs/defaults.c

[-- Attachment #3: patch-7-nobody-default.diff --]
[-- Type: text/x-patch, Size: 1934 bytes --]

This patch changes the default owning uid/gid for files
created by cdrwtool and mkudffs ( basically the root
and lost+found directories ) from 0 to -1.  This allows
the udf filesystem driver to obey the uid/gid mount
options.  When the FSD sees a -1, if the uid/gid mount
options were specified, it replaces the -1 with those
values.  

I also patched the FSD ( not in this patch )
to write a -1 if the owner matches the mount option
(previously it wrote a 0 instead, so files would
spontaniously change ownership when unmounted/mounted
to root if they were owned by the uid specified in
the mount options ).  This allows sane application
of the mount options to allow a non root user, or even
different users, possibly on different machines,  to use
the disc.  

 - Phillip Susi <psusi@cfl.rr.com>

diff -ru udftools-1.0.0b3.orig/cdrwtool/defaults.c udftools-1.0.0b3/cdrwtool/defaults.c
--- udftools-1.0.0b3.orig/cdrwtool/defaults.c	2002-11-26 02:18:50.000000000 -0500
+++ udftools-1.0.0b3/cdrwtool/defaults.c	2006-01-03 23:19:49.000000000 -0500
@@ -443,6 +443,8 @@
 			UDF_OS_ID_LINUX,
 		},
 	},
+	uid : -1,
+	gid : -1,
 };
 
 struct extendedFileEntry default_efe =
@@ -476,4 +478,6 @@
 			UDF_OS_ID_LINUX,
 		},
 	},
+	uid : -1,
+	gid : -1,
 };
diff -ru udftools-1.0.0b3.orig/cdrwtool/main.c udftools-1.0.0b3/cdrwtool/main.c
diff -ru udftools-1.0.0b3.orig/debian/changelog udftools-1.0.0b3/debian/changelog
diff -ru udftools-1.0.0b3.orig/doc/cdrwtool.1 udftools-1.0.0b3/doc/cdrwtool.1
diff -ru udftools-1.0.0b3.orig/mkudffs/defaults.c udftools-1.0.0b3/mkudffs/defaults.c
--- udftools-1.0.0b3.orig/mkudffs/defaults.c	2002-11-26 02:18:51.000000000 -0500
+++ udftools-1.0.0b3/mkudffs/defaults.c	2006-01-03 23:20:11.000000000 -0500
@@ -450,6 +450,8 @@
 			UDF_OS_ID_LINUX,
 		},
 	},
+	uid : -1,
+	gid : -1,
 };
 
 struct extendedFileEntry default_efe =
@@ -483,4 +485,6 @@
 			UDF_OS_ID_LINUX,
 		},
 	},
+	uid : -1,
+	gid : -1,
 };

[-- Attachment #4: patch-8-pktsetup-plugdev.diff --]
[-- Type: text/x-patch, Size: 3912 bytes --]

This patch adds a new single parameter form to pktsetup.  
This new form takes only the name of the cdrom device to
bind to, and will bind the next unused pktcdvd minor
device to it, and print that device number to stdout.  

 - Phillip Susi <psusi@cfl.rr.com>

diff -ru udftools-1.0.0b3.orig/doc/pktsetup.8 udftools-1.0.0b3/doc/pktsetup.8
--- udftools-1.0.0b3.orig/doc/pktsetup.8	2006-01-08 18:08:04.000000000 -0500
+++ udftools-1.0.0b3/doc/pktsetup.8	2006-01-08 18:58:59.000000000 -0500
@@ -37,14 +37,32 @@
 .B pktsetup
 .B \-d
 .I packet_device
+.br
+.B pktsetup
+.I /dev/cdrom
 .ad b
 .SH DESCRIPTION
-.B Pktsetup
+.B \fBpktsetup\fP
 is used to associate packet devices with CD or DVD block devices,
 so that the packet device can then be mounted and potentially
 used as a read/write filesystem. This requires kernel support for
 the packet device, and the UDF filesystem.
 .PP
+To old style to set up a device is:
+.IP
+\fBpktsetup \fI/dev/pktcdvd/pktcdvd0 /dev/cdrom\fP
+.PP
+This usage will create the devnode \fI/dev/pktcdvd/pktcdvd0\fP
+and bind it to \fI/dev/cdrom\fP.  
+.PP
+The new form is:
+.IP
+\fBpktsetup \fI/dev/cdrom\fP
+.PP
+This form will bind the next availible pkcdvd minor device to
+\fI/dev/cdrom\fP and print the device number to stdout.  No device
+node will be created, that job is left up to udev.  
+.PP
 See /usr/share/doc/udftools/README.Debian for more information.
 .UE
 
@@ -55,7 +73,9 @@
 .SH OPTIONS
 .IP "\fB\-d \fIpacket-device\fP"
 Delete the association between the specified \fIpacket-device\fP
-and its block device.
+and its block device.  The device may either be of the form
+\fI/dev/pktcdvd/pktcdvd0\fP or \fI252:0\fP.
+
 
 .SH EXAMPLE
 The following commands provide an example of using the
@@ -74,7 +94,7 @@
 
 .SH FILES
 .nf
-/dev/pktcdvd0,/dev/pktcdvd1,...  CD/DVD packet devices (block major=97)
+/dev/pktcdvd0,/dev/pktcdvd1,...  CD/DVD packet devices (block major=252)
 .fi
 
 .SH AUTHOR
diff -ru udftools-1.0.0b3.orig/pktsetup/pktsetup.c udftools-1.0.0b3/pktsetup/pktsetup.c
--- udftools-1.0.0b3.orig/pktsetup/pktsetup.c	2006-01-08 18:08:03.000000000 -0500
+++ udftools-1.0.0b3/pktsetup/pktsetup.c	2006-01-08 18:34:58.000000000 -0500
@@ -125,6 +125,8 @@
 	printf("  pktsetup -d dev_name               tear down device\n");
 	printf("  pktsetup -d major:minor            tear down device\n");
 	printf("  pktsetup -s                        show device mappings\n");
+	printf("And in this version:\n");
+	printf("  pktsetup /dev/cdrom                setup device\n");
 	return 1;
 }
 
@@ -245,7 +247,7 @@
 		c.command = PKT_CTRL_CMD_SETUP;
 		c.dev = stat_buf.st_rdev;
 
-		if (remove_stale_dev_node(ctl_fd, pkt_device) != 0) {
+		if (pkt_device && remove_stale_dev_node(ctl_fd, pkt_device) != 0) {
 			fprintf(stderr, "Device node '%s' already in use\n", pkt_device);
 			goto out_close;
 		}
@@ -253,7 +255,10 @@
 			perror("ioctl");
 			goto out_close;
 		}
-		mknod(pkt_dev_name(pkt_device), S_IFBLK | 0640, c.pkt_dev);
+		if( pkt_device )
+		  mknod(pkt_dev_name(pkt_device), S_IFBLK | 0640, c.pkt_dev);
+		else
+		  printf("Assigned device number %d:%d\n", MAJOR(c.pkt_dev), MINOR(c.pkt_dev) );
 	} else {
 		int major, minor, remove_node;
 
@@ -322,9 +327,6 @@
 	char *pkt_device;
 	char *device;
 
-	if (argc == 1)
-		return usage();
-
 	while ((c = getopt(argc, argv, "ds?")) != EOF) {
 		switch (c) {
 			case 'd':
@@ -337,11 +339,15 @@
 				return usage();
 		}
 	}
-	pkt_device = argv[optind];
-	device = argv[optind + 1];
-	if (strchr(pkt_device, '/'))
-		setup_dev(pkt_device, device, rem);
-	else
-		setup_dev_chardev(pkt_device, device, rem);
-	return 0;
+	if( argc == 2 )
+	    setup_dev_chardev(NULL, argv[1], 0 );
+	else {
+	  pkt_device = argv[optind];
+	  device = argv[optind + 1];
+	  if (strchr(pkt_device, '/'))
+	    setup_dev(pkt_device, device, rem);
+	  else
+	    setup_dev_chardev(pkt_device, device, rem);
+	  return 0;
+	}
 }

Re: [PATCH] pktcdvd & udf bugfixes

[Peter Osterlund]

Phillip Susi <psusi@cfl.rr.com> writes:

> Peter Osterlund wrote:
>   > OK, so it appears you can make packets bigger than 64KB. Can I please
> > have those patches so I can test this myself.
> 
> Sure, patches attached.
> 
> patch-6 is the one you are interested in for the different sizes

Thanks, it seems to work just fine. I have put the overflow and zero
check changes in my kernel patch queue.

However, I want to wait with the increased max packet size until the
memory allocation strategy has been changed to avoid wasting lots of
memory in the common cases. I will go work on a patch to do that now.

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] pktcdvd & udf bugfixes

[Phillip Susi]

Ahh, excellent.  Also, is the memory currently non pagable?  Is there a 
reason for that?

Peter Osterlund wrote:

>Phillip Susi <psusi@cfl.rr.com> writes:
>
>  
>
>>Peter Osterlund wrote:
>>  > OK, so it appears you can make packets bigger than 64KB. Can I please
>>    
>>
>>>have those patches so I can test this myself.
>>>      
>>>
>>Sure, patches attached.
>>
>>patch-6 is the one you are interested in for the different sizes
>>    
>>
>
>Thanks, it seems to work just fine. I have put the overflow and zero
>check changes in my kernel patch queue.
>
>However, I want to wait with the increased max packet size until the
>memory allocation strategy has been changed to avoid wasting lots of
>memory in the common cases. I will go work on a patch to do that now.
>
>  
>

Re: [PATCH] pktcdvd & udf bugfixes

[Peter Osterlund]

Phillip Susi <psusi@cfl.rr.com> writes:

> Peter Osterlund wrote:
> 
> >Phillip Susi <psusi@cfl.rr.com> writes:
> >
> >>Peter Osterlund wrote:
> >>  > OK, so it appears you can make packets bigger than 64KB. Can I please
> >>
> >>>have those patches so I can test this myself.
> >>>
> >>Sure, patches attached.
> >>
> >>patch-6 is the one you are interested in for the different sizes
> >
> >Thanks, it seems to work just fine. I have put the overflow and zero
> >check changes in my kernel patch queue.
> >
> >However, I want to wait with the increased max packet size until the
> >memory allocation strategy has been changed to avoid wasting lots of
> >memory in the common cases. I will go work on a patch to do that now.
> >
> Ahh, excellent.  Also, is the memory currently non pagable?  Is there
> a reason for that?

Yes the memory is non pagable. The linux kernel doesn't support
pagable kernel memory, only user space memory can be swapped out.

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] pktcdvd & udf bugfixes

[Phillip Susi]

Surely the kernel can allocate pagable memory if it chooses to?  I think 
in the case of pktcdvd it would be best for the buffers to be pagable 
when initially allocated and filled, and only locked into memory when 
the bio is sent down to the cdrom driver.  That way you could keep a 
fairly large number of buffers on larger ( 256 KB+ ) packets in memory 
for write combining, without placing undue burden on non paged pool. 


Peter Osterlund wrote:

>>Ahh, excellent.  Also, is the memory currently non pagable?  Is there
>>a reason for that?
>>    
>>
>
>Yes the memory is non pagable. The linux kernel doesn't support
>pagable kernel memory, only user space memory can be swapped out.
>
>  
>

Re: [PATCH] pktcdvd & udf bugfixes

[Peter Osterlund]

Peter Osterlund <petero2@telia.com> writes:

> Phillip Susi <psusi@cfl.rr.com> writes:
> 
> > Peter Osterlund wrote:
> >   > OK, so it appears you can make packets bigger than 64KB. Can I please
> > > have those patches so I can test this myself.
> > 
> > Sure, patches attached.
> > 
> > patch-6 is the one you are interested in for the different sizes
> 
> Thanks, it seems to work just fine. I have put the overflow and zero
> check changes in my kernel patch queue.
> 
> However, I want to wait with the increased max packet size until the
> memory allocation strategy has been changed to avoid wasting lots of
> memory in the common cases. I will go work on a patch to do that now.

Here is the patch. It works as expected so far in my tests.


pktcdvd: Don't waste kernel memory.

From: Peter Osterlund <petero2@telia.com>

Allocate memory for read-gathering at open time, when it is known just
how much memory is needed.  This avoids wasting kernel memory when the
real packet size is smaller than the maximum packet size supported by
the driver.

Signed-off-by: Peter Osterlund <petero2@telia.com>
---

 drivers/block/Kconfig   |    4 ++--
 drivers/block/pktcdvd.c |   53 +++++++++++++++++++++++++----------------------
 include/linux/pktcdvd.h |    4 ++--
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 139cbba..f7a051b 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -433,8 +433,8 @@ config CDROM_PKTCDVD_BUFFERS
 	  This controls the maximum number of active concurrent packets. More
 	  concurrent packets can increase write performance, but also require
 	  more memory. Each concurrent packet will require approximately 64Kb
-	  of non-swappable kernel memory, memory which will be allocated at
-	  pktsetup time.
+	  of non-swappable kernel memory, memory which will be allocated when
+	  a disc is opened for writing.
 
 config CDROM_PKTCDVD_WCACHE
 	bool "Enable write caching"
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index b8b561e..eea393d 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -132,7 +132,7 @@ static struct bio *pkt_bio_alloc(int nr_
 /*
  * Allocate a packet_data struct
  */
-static struct packet_data *pkt_alloc_packet_data(void)
+static struct packet_data *pkt_alloc_packet_data(int frames)
 {
 	int i;
 	struct packet_data *pkt;
@@ -141,11 +141,12 @@ static struct packet_data *pkt_alloc_pac
 	if (!pkt)
 		goto no_pkt;
 
-	pkt->w_bio = pkt_bio_alloc(PACKET_MAX_SIZE);
+	pkt->frames = frames;
+	pkt->w_bio = pkt_bio_alloc(frames);
 	if (!pkt->w_bio)
 		goto no_bio;
 
-	for (i = 0; i < PAGES_PER_PACKET; i++) {
+	for (i = 0; i < frames / FRAMES_PER_PAGE; i++) {
 		pkt->pages[i] = alloc_page(GFP_KERNEL|__GFP_ZERO);
 		if (!pkt->pages[i])
 			goto no_page;
@@ -153,7 +154,7 @@ static struct packet_data *pkt_alloc_pac
 
 	spin_lock_init(&pkt->lock);
 
-	for (i = 0; i < PACKET_MAX_SIZE; i++) {
+	for (i = 0; i < frames; i++) {
 		struct bio *bio = pkt_bio_alloc(1);
 		if (!bio)
 			goto no_rd_bio;
@@ -163,14 +164,14 @@ static struct packet_data *pkt_alloc_pac
 	return pkt;
 
 no_rd_bio:
-	for (i = 0; i < PACKET_MAX_SIZE; i++) {
+	for (i = 0; i < frames; i++) {
 		struct bio *bio = pkt->r_bios[i];
 		if (bio)
 			bio_put(bio);
 	}
 
 no_page:
-	for (i = 0; i < PAGES_PER_PACKET; i++)
+	for (i = 0; i < frames / FRAMES_PER_PAGE; i++)
 		if (pkt->pages[i])
 			__free_page(pkt->pages[i]);
 	bio_put(pkt->w_bio);
@@ -187,12 +188,12 @@ static void pkt_free_packet_data(struct 
 {
 	int i;
 
-	for (i = 0; i < PACKET_MAX_SIZE; i++) {
+	for (i = 0; i < pkt->frames; i++) {
 		struct bio *bio = pkt->r_bios[i];
 		if (bio)
 			bio_put(bio);
 	}
-	for (i = 0; i < PAGES_PER_PACKET; i++)
+	for (i = 0; i < pkt->frames / FRAMES_PER_PAGE; i++)
 		__free_page(pkt->pages[i]);
 	bio_put(pkt->w_bio);
 	kfree(pkt);
@@ -207,17 +208,17 @@ static void pkt_shrink_pktlist(struct pk
 	list_for_each_entry_safe(pkt, next, &pd->cdrw.pkt_free_list, list) {
 		pkt_free_packet_data(pkt);
 	}
+	INIT_LIST_HEAD(&pd->cdrw.pkt_free_list);
 }
 
 static int pkt_grow_pktlist(struct pktcdvd_device *pd, int nr_packets)
 {
 	struct packet_data *pkt;
 
-	INIT_LIST_HEAD(&pd->cdrw.pkt_free_list);
-	INIT_LIST_HEAD(&pd->cdrw.pkt_active_list);
-	spin_lock_init(&pd->cdrw.active_list_lock);
+	BUG_ON(!list_empty(&pd->cdrw.pkt_free_list));
+
 	while (nr_packets > 0) {
-		pkt = pkt_alloc_packet_data();
+		pkt = pkt_alloc_packet_data(pd->settings.size >> 2);
 		if (!pkt) {
 			pkt_shrink_pktlist(pd);
 			return 0;
@@ -952,7 +953,7 @@ try_next_bio:
 
 	pd->current_sector = zone + pd->settings.size;
 	pkt->sector = zone;
-	pkt->frames = pd->settings.size >> 2;
+	BUG_ON(pkt->frames != pd->settings.size >> 2);
 	pkt->write_size = 0;
 
 	/*
@@ -1988,8 +1989,14 @@ static int pkt_open_dev(struct pktcdvd_d
 	if ((ret = pkt_set_segment_merging(pd, q)))
 		goto out_unclaim;
 
-	if (write)
+	if (write) {
+		if (!pkt_grow_pktlist(pd, CONFIG_CDROM_PKTCDVD_BUFFERS)) {
+			printk("pktcdvd: not enough memory for buffers\n");
+			ret = -ENOMEM;
+			goto out_unclaim;
+		}
 		printk("pktcdvd: %lukB available on disc\n", lba << 1);
+	}
 
 	return 0;
 
@@ -2015,6 +2022,8 @@ static void pkt_release_dev(struct pktcd
 	pkt_set_speed(pd, MAX_SPEED, MAX_SPEED);
 	bd_release(pd->bdev);
 	blkdev_put(pd->bdev);
+
+	pkt_shrink_pktlist(pd);
 }
 
 static struct pktcdvd_device *pkt_find_dev_from_minor(int dev_minor)
@@ -2380,12 +2389,6 @@ static int pkt_new_dev(struct pktcdvd_de
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
 
-	if (!pkt_grow_pktlist(pd, CONFIG_CDROM_PKTCDVD_BUFFERS)) {
-		printk("pktcdvd: not enough memory for buffers\n");
-		ret = -ENOMEM;
-		goto out_mem;
-	}
-
 	pd->bdev = bdev;
 	set_blocksize(bdev, CD_FRAMESIZE);
 
@@ -2396,7 +2399,7 @@ static int pkt_new_dev(struct pktcdvd_de
 	if (IS_ERR(pd->cdrw.thread)) {
 		printk("pktcdvd: can't start kernel thread\n");
 		ret = -ENOMEM;
-		goto out_thread;
+		goto out_mem;
 	}
 
 	proc = create_proc_entry(pd->name, 0, pkt_proc);
@@ -2407,8 +2410,6 @@ static int pkt_new_dev(struct pktcdvd_de
 	DPRINTK("pktcdvd: writer %s mapped to %s\n", pd->name, bdevname(bdev, b));
 	return 0;
 
-out_thread:
-	pkt_shrink_pktlist(pd);
 out_mem:
 	blkdev_put(bdev);
 	/* This is safe: open() is still holding a reference. */
@@ -2504,6 +2505,10 @@ static int pkt_setup_dev(struct pkt_ctrl
 		goto out_mem;
 	pd->disk = disk;
 
+	INIT_LIST_HEAD(&pd->cdrw.pkt_free_list);
+	INIT_LIST_HEAD(&pd->cdrw.pkt_active_list);
+	spin_lock_init(&pd->cdrw.active_list_lock);
+
 	spin_lock_init(&pd->lock);
 	spin_lock_init(&pd->iosched.lock);
 	sprintf(pd->name, "pktcdvd%d", idx);
@@ -2568,8 +2573,6 @@ static int pkt_remove_dev(struct pkt_ctr
 
 	blkdev_put(pd->bdev);
 
-	pkt_shrink_pktlist(pd);
-
 	remove_proc_entry(pd->name, pkt_proc);
 	DPRINTK("pktcdvd: writer %s unmapped\n", pd->name);
 
diff --git a/include/linux/pktcdvd.h b/include/linux/pktcdvd.h
index 0346d6c..8a94c71 100644
--- a/include/linux/pktcdvd.h
+++ b/include/linux/pktcdvd.h
@@ -170,7 +170,7 @@ struct packet_iosched
 #error "PAGE_SIZE must be a multiple of CD_FRAMESIZE"
 #endif
 #define PACKET_MAX_SIZE		128
-#define PAGES_PER_PACKET	(PACKET_MAX_SIZE * CD_FRAMESIZE / PAGE_SIZE)
+#define FRAMES_PER_PAGE		(PAGE_SIZE / CD_FRAMESIZE)
 #define PACKET_MAX_SECTORS	(PACKET_MAX_SIZE * CD_FRAMESIZE >> 9)
 
 enum packet_data_state {
@@ -219,7 +219,7 @@ struct packet_data
 	atomic_t		io_errors;	/* Number of read/write errors during IO */
 
 	struct bio		*r_bios[PACKET_MAX_SIZE]; /* bios to use during data gathering */
-	struct page		*pages[PAGES_PER_PACKET];
+	struct page		*pages[PACKET_MAX_SIZE / FRAMES_PER_PAGE];
 
 	int			cache_valid;	/* If non-zero, the data for the zone defined */
 						/* by the sector variable is completely cached */

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] pktcdvd & udf bugfixes

[Peter Osterlund]

Phillip Susi <psusi@cfl.rr.com> writes:

> >>Ahh, excellent.  Also, is the memory currently non pagable?  Is there
> >>a reason for that?
> >
> >Yes the memory is non pagable. The linux kernel doesn't support
> >pagable kernel memory, only user space memory can be swapped out.
> 
> Surely the kernel can allocate pagable memory if it chooses to?

No, not unless you make large changes in the VM subsystem.

But there is another issue here as well. Even if the kernel could
allocate pagable memory, it would be non-trivial to use it in a block
device driver. A block driver can be asked by the kernel to write out
memory to disc *because* there is memory pressure in the system. If
the block driver needs to make additional memory allocations before it
can write data (for example, to page in swapped out memory), the
system could deadlock.

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] pktcdvd & udf bugfixes

[Phillip Susi]

I get this:

drivers/block/pktcdvd.c:1992: error: label ‘out_unclaim’ used but not 
defined

Also the patch did not cleanly apply. It looks like you didn't get all 
of your changes into the diff. Unless you have some other patches that I 
don't? This is against 2.6.15 right?

Peter Osterlund wrote:

>Here is the patch. It works as expected so far in my tests.
>
>
>pktcdvd: Don't waste kernel memory.
>
>From: Peter Osterlund <petero2@telia.com>
>
>Allocate memory for read-gathering at open time, when it is known just
>how much memory is needed.  This avoids wasting kernel memory when the
>real packet size is smaller than the maximum packet size supported by
>the driver.
>
>Signed-off-by: Peter Osterlund <petero2@telia.com>
>---
>
>  
>

Re: [PATCH] pktcdvd & udf bugfixes

[Peter Osterlund]

Phillip Susi <psusi@cfl.rr.com> writes:

> I get this:
> 
> drivers/block/pktcdvd.c:1992: error: label ‘out_unclaim’ used but
> not defined
> 
> Also the patch did not cleanly apply. It looks like you didn't get all
> of your changes into the diff. Unless you have some other patches that
> I don't? This is against 2.6.15 right?

I thought it would apply on top of your patch, but didn't actually
check. Try this patch series instead:

        http://web.telia.com/~u89404340/patches/packet/2.6/2.6.15-git11/

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] pktcdvd & udf bugfixes

[Phillip Susi]

Well, I got a kernel compiled with your patches, but have not been able 
to test it yet. Last week the red laser crapped out on my sony 710A 
dvd/rw drive, so I got a new 810A and when I go to format the cd-rw with 
cdrwtool, the drive fails the write command with sense code 00.04.04, 
and then fails any commands after that and won't eject the disc until I 
do a hdparm -w. I guess the firmware is defective and craps itself when 
you try to do a packet mode write ( updating didn't help ), so I'm 
returning this drive and ordering the nice sata plextor one instead.

Will let you know when I get the new drive if your patch works out well 
or not.

Peter Osterlund wrote:
> Phillip Susi <psusi@cfl.rr.com> writes:
>
>   
>> I get this:
>>
>> drivers/block/pktcdvd.c:1992: error: label ‘out_unclaim’ used but
>> not defined
>>
>> Also the patch did not cleanly apply. It looks like you didn't get all
>> of your changes into the diff. Unless you have some other patches that
>> I don't? This is against 2.6.15 right?
>>     
>
> I thought it would apply on top of your patch, but didn't actually
> check. Try this patch series instead:
>
>         http://web.telia.com/~u89404340/patches/packet/2.6/2.6.15-git11/
>
>