From owner-freebsd-hackers@FreeBSD.ORG Fri Feb 19 18:24:11 2010 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5B5B71065676; Fri, 19 Feb 2010 18:24:11 +0000 (UTC) (envelope-from nox@jelal.kn-bremen.de) Received: from smtp.kn-bremen.de (gelbbaer.kn-bremen.de [78.46.108.116]) by mx1.freebsd.org (Postfix) with ESMTP id E841A8FC3F; Fri, 19 Feb 2010 18:24:10 +0000 (UTC) Received: by smtp.kn-bremen.de (Postfix, from userid 10) id 7418C1E00772; Fri, 19 Feb 2010 19:24:09 +0100 (CET) Received: from triton8.kn-bremen.de (noident@localhost [127.0.0.1]) by triton8.kn-bremen.de (8.14.3/8.14.3) with ESMTP id o1JIClsf035888; Fri, 19 Feb 2010 19:12:47 +0100 (CET) (envelope-from nox@triton8.kn-bremen.de) Received: (from nox@localhost) by triton8.kn-bremen.de (8.14.3/8.14.3/Submit) id o1JIClYS035887; Fri, 19 Feb 2010 19:12:47 +0100 (CET) (envelope-from nox) From: Juergen Lock Date: Fri, 19 Feb 2010 19:12:47 +0100 To: Garrett Cooper Message-ID: <20100219181247.GA35702@triton8.kn-bremen.de> References: <20100217215940.GA19713@triton8.kn-bremen.de> <4B7CE066.4030403@freebsd.org> <20100218183459.GA65508@triton8.kn-bremen.de> <7d6fde3d1002182212k3bdc866ckdc7c5f380b40f7d8@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <7d6fde3d1002182212k3bdc866ckdc7c5f380b40f7d8@mail.gmail.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-Mailman-Approved-At: Fri, 19 Feb 2010 18:36:11 +0000 Cc: freebsd-hackers@freebsd.org, Tim Kientzle , Juergen Lock Subject: Re: "tar tfv /dev/cd0" speedup patch X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Feb 2010 18:24:11 -0000 On Thu, Feb 18, 2010 at 10:12:24PM -0800, Garrett Cooper wrote: > On Thu, Feb 18, 2010 at 10:34 AM, Juergen Lock wrote: > > On Wed, Feb 17, 2010 at 10:38:30PM -0800, Tim Kientzle wrote: > >> Juergen Lock wrote: > >> > > >> >  ...  since bsdtar/libarchive know iso9660 I just did the command in the > >> > Subject.  It worked, but it was sloow... :(  Apparently it read all of > >> > the disc without seeking.  The following patch fixes this, is something > >> > like this desired?  If yes I could look how to do the same for Linux, > >> > >> Juergen, > >> > >> This is great!  If you can figure out how to get this > >> right, I would really appreciate it.  If you have a > >> tape drive handy, definitely test with that.  My first > >> attempts here actually broke reading from tape drives, > >> which is why the current code is so conservative. > >> > > Hmm I can't test on a tape atm but if I look at the kernel, > >        http://fxr.watson.org/fxr/ident?i=DIOCGMEDIASIZE > > DIOCGMEDIASIZE is only handled for geom, xen block devices, old CD drives > > and pc98 floppies, and I'm pretty sure tapes don't use geom. :) > > > >> Minor style comments: > >> >  else if (S_ISCHR(st.st_mode) && > >> >     !ioctl(fd, DIOCGMEDIASIZE, &mediasize) && mediasize) { > >> > >> Please be explicit:  S_ISCHR() && ioctl() == 0  && mediasize > 0 > >> > >> >     archive_read_extract_set_skip_file(a, st.st_dev, st.st_ino); > >> > >> extract_skip_file isn't needed here; we don't read the > >> contents of device nodes. > >> > >> Let me know as soon as you have something you're confident of. > > > >  Ok here is a new version of the patch with these things fixed and the > > Linux case added:  (Linux case not tested yet, and yes I did this on > > stable/8.) > > > > Index: src/lib/libarchive/archive_read_open_filename.c > > =================================================================== > > RCS file: /home/scvs/src/lib/libarchive/archive_read_open_filename.c,v > > retrieving revision 1.25.2.1 > > diff -u -p -r1.25.2.1 archive_read_open_filename.c > > --- src/lib/libarchive/archive_read_open_filename.c     3 Aug 2009 08:13:06 -0000       1.25.2.1 > > +++ src/lib/libarchive/archive_read_open_filename.c     18 Feb 2010 18:14:16 -0000 > > @@ -44,6 +44,10 @@ __FBSDID("$FreeBSD: src/lib/libarchive/a > >  #ifdef HAVE_UNISTD_H > >  #include > >  #endif > > +#ifdef __FreeBSD__ > > +#include > > +#include > > +#endif > > > >  #include "archive.h" > > > > @@ -83,6 +87,9 @@ archive_read_open_filename(struct archiv > >        struct read_file_data *mine; > >        void *b; > >        int fd; > > +#ifdef __FreeBSD__ > > +       off_t mediasize = 0; > > +#endif > > > >        archive_clear_error(a); > >        if (filename == NULL || filename[0] == '\0') { > > @@ -143,6 +150,27 @@ archive_read_open_filename(struct archiv > >                 */ > >                mine->can_skip = 1; > >        } > > +#ifdef __FreeBSD__ > > +       /* > > +        * on FreeBSD if a device supports the DIOCGMEDIASIZE ioctl > > +        * it is a disk-like device and should be seekable. > > +        */ > > +       else if (S_ISCHR(st.st_mode) && > > +           ioctl(fd, DIOCGMEDIASIZE, &mediasize) == 0 && mediasize > 0) { > > +               mine->can_skip = 1; > > +       } > > +#endif > > +#ifdef __linux__ > > Is there any reason why a) you wouldn't check for other BSDs? b) you > aren't doing something of the flavor... > > #if defined(__FreeBSD__) > > #elif defined(__linux__) > > #endif Okok, here's a new version of the patch that now also knows about other BSDs and switched to #elif: (only NetBSD actually tested tho since I had a VM lying around, more testing welcome!) (Also if anyone wants to switch this to autoconf-style HAVE_FOO handling they are welcome too... :) Enjoy, Juergen Index: src/lib/libarchive/archive_read_open_filename.c =================================================================== RCS file: /home/scvs/src/lib/libarchive/archive_read_open_filename.c,v retrieving revision 1.25.2.1 diff -u -p -u -p -r1.25.2.1 archive_read_open_filename.c --- src/lib/libarchive/archive_read_open_filename.c 3 Aug 2009 08:13:06 -0000 1.25.2.1 +++ src/lib/libarchive/archive_read_open_filename.c 19 Feb 2010 18:18:24 -0000 @@ -44,6 +44,17 @@ __FBSDID("$FreeBSD: src/lib/libarchive/a #ifdef HAVE_UNISTD_H #include #endif +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) +#include +#include +#elif defined(__NetBSD__) || defined(__OpenBSD__) +#include +#include +#include +#elif defined(__DragonFly__) +#include +#include +#endif #include "archive.h" @@ -83,6 +94,13 @@ archive_read_open_filename(struct archiv struct read_file_data *mine; void *b; int fd; +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + off_t mediasize = 0; +#elif defined(__NetBSD__) || defined(__OpenBSD__) + struct disklabel dl; +#elif defined(__DragonFly__) + struct partinfo pi; +#endif archive_clear_error(a); if (filename == NULL || filename[0] == '\0') { @@ -143,6 +161,45 @@ archive_read_open_filename(struct archiv */ mine->can_skip = 1; } +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + /* + * on FreeBSD if a device supports the DIOCGMEDIASIZE ioctl + * it is a disk-like device and should be seekable. + */ + else if (S_ISCHR(st.st_mode) && + ioctl(fd, DIOCGMEDIASIZE, &mediasize) == 0 && mediasize > 0) { + mine->can_skip = 1; + } +#elif defined(__NetBSD__) || defined(__OpenBSD__) + /* + * on Net/OpenBSD if a device supports the DIOCGDINFO ioctl + * it is a disk-like device and should be seekable. + */ + else if ((S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) && + ioctl(fd, DIOCGDINFO, &dl) == 0 && + dl.d_partitions[DISKPART(st.st_rdev)].p_size > 0) { + mine->can_skip = 1; + } +#elif defined(__DragonFly__) + /* + * on DragonFly BSD if a device supports the DIOCGPART ioctl + * it is a disk-like device and should be seekable. + */ + else if (S_ISCHR(st.st_mode) && + ioctl(fd, DIOCGPART, &pi) == 0 && pi.media_size > 0) { + mine->can_skip = 1; + } +#elif defined(__linux__) + /* + * on Linux just check whether its a block device and that + * lseek works. (Tapes are character devices there.) + */ + else if (S_ISBLK(st.st_mode) && + lseek(fd, 0, SEEK_CUR) == 0 && lseek(fd, 0, SEEK_SET) == 0 && + lseek(fd, 0, SEEK_END) > 0 && lseek(fd, 0, SEEK_SET) == 0) { + mine->can_skip = 1; + } +#endif return (archive_read_open2(a, mine, NULL, file_read, file_skip, file_close)); }