Date: Fri, 19 Feb 2010 21:20:30 -0800 From: Tim Kientzle <kientzle@freebsd.org> To: Juergen Lock <nox@jelal.kn-bremen.de> Cc: Garrett Cooper <yanefbsd@gmail.com>, freebsd-hackers@freebsd.org Subject: Re: "tar tfv /dev/cd0" speedup patch Message-ID: <4B7F711E.6040402@freebsd.org> In-Reply-To: <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> <20100219181247.GA35702@triton8.kn-bremen.de>
next in thread | previous in thread | raw e-mail | index | archive | help
Juergen,
I was looking at your Linux code here and thought
the technique of trying lseek(SEEK_END) might work.
Unfortunately, it doesn't: lseek(fd, 0, SEEK_END) gives
zero for both /dev/sa0 (a tape drive) and /dev/cd0
(an optical drive). Are you sure it works on Linux?
Tim
P.S. Here's the program I used to test. Running it
against regular files and various device
nodes is quite illuminating.
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
int
main(int argc, char **argv)
{
int fd;
if (argv[1] == NULL) {
fprintf(stderr, "Need to specify a pathname.\n");
exit(1);
}
fd = open(argv[1], O_RDWR);
printf("fd=%d\n", fd);
printf("lseek(fd, 0, SEEK_SET)=%d\n",
(int)lseek(fd, 0, SEEK_SET));
printf("lseek(fd, 10240, SEEK_SET)=%d\n",
(int)lseek(fd, 10240, SEEK_SET));
printf("lseek(fd, 10240, SEEK_CUR)=%d\n",
(int)lseek(fd, 10240, SEEK_CUR));
printf("lseek(fd, 0, SEEK_END)=%d\n",
(int)lseek(fd, 0, SEEK_END));
printf("lseek(fd, 0, SEEK_SET)=%d\n",
(int)lseek(fd, 0, SEEK_SET));
return (0);
}
Juergen Lock wrote:
> On Thu, Feb 18, 2010 at 10:12:24PM -0800, Garrett Cooper wrote:
>> On Thu, Feb 18, 2010 at 10:34 AM, Juergen Lock <nox@jelal.kn-bremen.de> 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 <unistd.h>
>>> #endif
>>> +#ifdef __FreeBSD__
>>> +#include <sys/ioctl.h>
>>> +#include <sys/disk.h>
>>> +#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 <unistd.h>
> #endif
> +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> +#include <sys/ioctl.h>
> +#include <sys/disk.h>
> +#elif defined(__NetBSD__) || defined(__OpenBSD__)
> +#include <sys/ioctl.h>
> +#include <sys/disklabel.h>
> +#include <sys/dkio.h>
> +#elif defined(__DragonFly__)
> +#include <sys/ioctl.h>
> +#include <sys/diskslice.h>
> +#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));
> }
>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4B7F711E.6040402>