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.
Hi,
On 1/12/06, Phillip Susi <[email protected]> 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
Phillip Susi <[email protected]> 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 - [email protected]
http://web.telia.com/~u89404340
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.
Phillip Susi <[email protected]> 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 - [email protected]
http://web.telia.com/~u89404340
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.
Phillip Susi <[email protected]> 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 - [email protected]
http://web.telia.com/~u89404340
Ahh, excellent. Also, is the memory currently non pagable? Is there a
reason for that?
Peter Osterlund wrote:
>Phillip Susi <[email protected]> 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.
>
>
>
Phillip Susi <[email protected]> writes:
> Peter Osterlund wrote:
>
> >Phillip Susi <[email protected]> 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 - [email protected]
http://web.telia.com/~u89404340
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.
>
>
>
Peter Osterlund <[email protected]> writes:
> Phillip Susi <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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 - [email protected]
http://web.telia.com/~u89404340
Phillip Susi <[email protected]> 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 - [email protected]
http://web.telia.com/~u89404340
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 <[email protected]>
>
>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 <[email protected]>
>---
>
>
>
Phillip Susi <[email protected]> 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 - [email protected]
http://web.telia.com/~u89404340
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 <[email protected]> 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/
>
>