growisofs buffer size does not respect bufsize option

Bug #162736 reported by Miklos Juhasz
8
Affects Status Importance Assigned to Milestone
dvd+rw-tools (Debian)
Fix Released
Unknown
dvd+rw-tools (Ubuntu)
Fix Released
Medium
Martin Pitt

Bug Description

Binary package hint: dvd+rw-tools

When burning DVDs, growisofs is not honoring the --use-the-force-luke=bufsize:X option, allocating an internal ring buffer ridiculously small, so that DVD burning is affected by frequent buffer underruns (the speed drops from 8x to as low as 2x).
The ring buffer is not of constant size: it varies for each burn, even though k3b informs -use-the-force-luke=bufsize:128M

I checked the code myself and found out that from version 7 growisofs tries limit the buffer size to a maximum of 1/4 of the RAM. In the source _SC_AVPHYS_PAGES returns the number of available, really free memory pages, not the total number of memory pages. Linux tends to use all available memory with buffers and cache, and keep just a bare minimum of free pages during normal use.

So, the code cuts the size of the buffer in half again and again until it is less or equal to 1/4 size of free memory (usually a few MB, 32 or 64 MB at most).

Andy Polyakov (author of growisofs) did not answer my e-mail last year so I patched growisofs myself. Later Fedora applied the same patch I did before.

I simply replaced _SC_AVPHYS_PAGES with _SC_PHYS_PAGES (total number of pages of physical memory in system). I have been using this modified growisofs for a year and no problem arised at all. It might have been a typo only...

--- growisofs.c.bak 2007-01-21 12:56:05.000000000 +0100
+++ growisofs.c 2007-01-21 13:00:02.000000000 +0100
@@ -3096,12 +3096,12 @@

 #if defined(__unix) || defined(__unix__)

-#if defined(_SC_PAGESIZE) && defined(_SC_AVPHYS_PAGES)
- { size_t phys_mem = (size_t)sysconf(_SC_AVPHYS_PAGES) *
+#if defined(_SC_PAGESIZE) && defined(_SC_PHYS_PAGES)
+ { size_t phys_mem = (size_t)sysconf(_SC_PHYS_PAGES) *
                        (size_t)sysconf(_SC_PAGESIZE);

        if (phys_mem)
- { phys_mem /= 2; /* normally AVPHYS is a bit smaller, so
+ { phys_mem /= 2; /* normally PHYS is a bit smaller, so
                             * we commonly land on 1/4 RAM */
            while (the_buffer_size > phys_mem) the_buffer_size /= 2;
        }

Tags: patch

Related branches

Revision history for this message
Luis Medinas (lmedinas) wrote :

Yes that patch makes sense and it shouldn't have any incompatibility problem. I'll build new packages with this patch and attach here.

Daniel Hahler (blueyed)
Changed in dvd+rw-tools:
importance: Undecided → Medium
status: New → Triaged
Revision history for this message
Miklos Juhasz (mjuhasz) wrote :

Will this patch make it into Hardy?

Revision history for this message
Daniel Hahler (blueyed) wrote :

To get your fix included in Ubuntu, it would help if you tried transforming it into a debdiff (http://wiki.ubuntu.com/PackagingGuide/Recipes/Debdiff) and submit it for review (http://wiki.ubuntu.com/SponsorshipProcess). If you prefer somebody else to do that, that's fine - please just indicate if you're available to do that.

Revision history for this message
Miklos Juhasz (mjuhasz) wrote :

Please find the debdiff attached. Could you please submit it for review?

Revision history for this message
Daniel Hahler (blueyed) wrote :

Thank you for your debdiff.
Please fix the following issues with it:
 - Use the existing patch system of the package (dpatch) for your patch (see dpatch-edit-patch). (You can use what-patch from ubuntu-dev-tools to find the used patch system)
 - Refer to the launchpad bug in the changelog entry ("LP: #162736")
 - Original-Maintainer should be "Ubuntu Core Developers <email address hidden>", and please mention the change in the changelog (see "update-maintainer" in ubuntu-dev-tools, you need to use "update-maintainer --section=main" currently)

Revision history for this message
Miklos Juhasz (mjuhasz) wrote :

Thank you for checking the debdiff.
Please find the new patch attached. This time I used dpatch and referred to the LP bug in the changelog. As for the maintainer definition, I got "Not an Ubuntu package or already maintained by the Ubuntu team." message. If there is still something to fix just let me know.

Revision history for this message
Hanno Stock (hefe_bia) (hanno-stock) wrote :

Hi Miklós,

thanks for your Debdiff. I'm not a MOTU, but AFAIK the contents of the Maintainer- and the XSBC-Original-Maintainer-Field have to be swapped. Also please update your change log entry to hardy as the distribution.
Otherwise it looks fine to me.

After updating your debdiff check for compliance with https://wiki.ubuntu.com/MOTU/Sponsorship/SponsorsQueue and the subscribe the bug to the ubuntu-main-sponsors team. (Most importantly set status to confirmed and assigned to nobody)

Revision history for this message
Miklos Juhasz (mjuhasz) wrote :

Patch modified according to the comments.

Changed in dvd+rw-tools:
status: Triaged → Confirmed
Changed in dvd+rw-tools:
status: Unknown → New
Revision history for this message
Daniel Holbach (dholbach) wrote :

Martin: can you check this out?

Martin Pitt (pitti)
Changed in dvd+rw-tools:
assignee: nobody → pitti
status: Confirmed → In Progress
Revision history for this message
Martin Pitt (pitti) wrote :

I fixed the debdiff a bit:

 * Add new dpatch to debian/patches/00list (otherwise it does not get applied)
 * Fix the patch to actually apply
 * Add a patch description
 * Tweak changelog a bit.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package dvd+rw-tools - 7.0-9ubuntu1

---------------
dvd+rw-tools (7.0-9ubuntu1) hardy; urgency=low

  [ Miklos Juhasz ]
  * Added patch to make growisofs respect the bufsize option (LP: #162736).

  [ Martin Pitt ]
  * Actually add the patch to debian/patches/00list, add a patch header, and
    fix the patch to actually apply.

 -- Martin Pitt <email address hidden> Wed, 02 Apr 2008 12:42:55 +0200

Changed in dvd+rw-tools:
status: In Progress → Fix Released
Changed in dvd+rw-tools:
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.