Re: Linux 2.6.20-rc1

Re: Linux 2.6.20-rc1

[Robert Hancock]

Alistair John Strachan wrote:
> I bisected all the way down to 0e75f9063f5c55fb0b0b546a7c356f8ec186825e, which 
> git reckons is the culprit. I wasn't able to revert this commit to test, 
> because it has conflicts.
> 
> Any ideas?

That would be this one I assume?

[PATCH] block: support larger block pc requests

author	Mike Christie <michaelc@cs.wisc.edu>
	Fri, 1 Dec 2006 09:40:55 +0000 (10:40 +0100)
committer	Jens Axboe <jens.axboe@oracle.com>
	Fri, 1 Dec 2006 09:40:55 +0000 (10:40 +0100)
commit	0e75f9063f5c55fb0b0b546a7c356f8ec186825e
tree	db138f641175403546c2147def4b405f3ff453a8
parent	ad2d7225709b11da47e092634cbdf0591829ae9c
[PATCH] block: support larger block pc requests

This patch modifies blk_rq_map/unmap_user() and the cdrom and scsi_ioctl.c
users so that it supports requests larger than bio by chaining them 
together.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Linux 2.6.20-rc1

[Linus Torvalds]

Ok, the two-week merge period is over, and -rc1 is out there.

I'm _really_ hoping that we can keep the 2.6.20 release calmer and without 
any of the dragging-out-due-to-core-changes that we've had lately. We 
didn't actually merge any really core changes here, with the biggest 
conceptual one being the "work_struct" split into regular work and 
"delayed" work, so I'm hoping we can really end up with an easy 2.6.20 
release.

Some of the commits there are pretty big patches, but more than a couple 
of them are due to fairly straightforward search-and-replace things (like 
a largely scripted removal of unnecessary casts of the return value of 
"kmalloc()", for example, or the switch to "ktermios" for the tty layer, 
or the introduction of "struct path" in the VFS layer instead of keeping 
the f_{dentry,vfsmnt} entries separate, or indeed the removal of SLAB_xxx 
constant names in favour of the standard GFP_xxx ones).

So while the patch itself isn't actually all that much smaller than usual, 
at least my personal gut feel is that the actual changes are not as 
intrusive, just in some cases have big diffs.

But both the diffstat and the shortlog are still too big to fit in the 
kernel mailing list limits, so you'll just have to take my word for it. Or 
get the git repo, and do your own delving into things with

	git log v2.6.19..v2.6.20-rc1 | git shortlog

There _are_ a few areas of note:

 - the aforementioned "workqueue" changes (where we still have some work 
   to do to finalize the proper actions on all architectures: it's being 
   somewhat discussed on the arch mailing lists, hopefully we'll have it 
   all resolved by -rc2, and it doesn't really worry me)

 - lockless page cache (RCU lookups of radix trees)

 - kvm driver for all those crazy virtualization people to play with

 - networking updates (DCCP, address-family agnostic connection tracking 
   in netfilter, sparse byte order annotations, yadda yadda)

 - HID layer separated out of the USB stuff (bluetooth apparently wants 
   the HID stuff too)

 - tons and tons of driver (ftape removal, ATA, pcmcia, i2c, 
   infiniband, dvb, networking..) and architecture updates (arm, mips, 
   powerpc, sh)

and probably some I just forgot about entirely.

		Linus

Re: Linux 2.6.20-rc1

[Gene Heskett]

On Wednesday 13 December 2006 21:06, Linus Torvalds wrote:
>Ok, the two-week merge period is over, and -rc1 is out there.
>
Ok, one not so silly Q (IMO) from the resident old fart.  I saw, sometime 
in the past week, a relatively huge ieee1394 update go by.  And I have 
some issues with the present 2.6.19 version causeing segfaults and kino 
go-aways when trying to capture from my firewire movie camera.  Problems 
occur when trying to control the camera from kino.

Is this patchset in this -rc1?  If it is, I'll see if I can get a build to 
work and check it out.

Thanks.

-- 
Cheers, Gene
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
Yahoo.com and AOL/TW attorneys please note, additions to the above
message by Gene Heskett are:
Copyright 2006 by Maurice Eugene Heskett, all rights reserved.

Re: Linux 2.6.20-rc1

[Linus Torvalds]

On Wed, 13 Dec 2006, Gene Heskett wrote:
>
> Ok, one not so silly Q (IMO) from the resident old fart.  I saw, sometime 
> in the past week, a relatively huge ieee1394 update go by.  And I have 
> some issues with the present 2.6.19 version causeing segfaults and kino 
> go-aways when trying to capture from my firewire movie camera.  Problems 
> occur when trying to control the camera from kino.
> 
> Is this patchset in this -rc1?  If it is, I'll see if I can get a build to 
> work and check it out.

-rc1 does include a reasonably big firewire update, but I'm not sure how 
it will affect your camera usage. In fact, the ieee1394 people seem to be 
trying to deprecate the dv1394 stuff, in favour of just raw1394 and 
user-mode libraries.

I think you can tell Kino to use either the DV or the raw interface, but 
I'm not sure. If you can, try either. The raw interface seems to be 
horribly misdesigned (security problems), but is the one to use.

But by all means, give it a shot, and regardless of whether it works 
better or not, you might want to cry on the shoulder of Stefan Richter 
<stefanr@s5r6.in-berlin.de> about the issues you see.. Of course, please 
talk to the Kino people too (although I have absolutely no idea who they 
would be).

		Linus

Re: Linux 2.6.20-rc1

[Gene Heskett]

On Wednesday 13 December 2006 22:32, Linus Torvalds wrote:
>On Wed, 13 Dec 2006, Gene Heskett wrote:
>> Ok, one not so silly Q (IMO) from the resident old fart.  I saw,
>> sometime in the past week, a relatively huge ieee1394 update go by. 
>> And I have some issues with the present 2.6.19 version causeing
>> segfaults and kino go-aways when trying to capture from my firewire
>> movie camera.  Problems occur when trying to control the camera from
>> kino.
>>
>> Is this patchset in this -rc1?  If it is, I'll see if I can get a
>> build to work and check it out.
>
>-rc1 does include a reasonably big firewire update, but I'm not sure how
>it will affect your camera usage. In fact, the ieee1394 people seem to
> be trying to deprecate the dv1394 stuff, in favour of just raw1394 and
> user-mode libraries.
>
>I think you can tell Kino to use either the DV or the raw interface, but
>I'm not sure. If you can, try either. The raw interface seems to be
>horribly misdesigned (security problems), but is the one to use.

It is using the raw stuff now.

>
>But by all means, give it a shot,

Will do, sometime in the next few days, its muzzleloading deer season here 
ATM.  And I wasted half the season on a tv station up in upstate MI.

>and regardless of whether it works 
>better or not, you might want to cry on the shoulder of Stefan Richter
><stefanr@s5r6.in-berlin.de> about the issues you see.. Of course, please
>talk to the Kino people too (although I have absolutely no idea who they
>would be).
>
>		Linus

That would be primarily Dan Dennedy although Stefan may be the next in 
line.  Dan tells us that kino will be finalized very shortly as he has 
other irons in the fire too.

-- 
Cheers, Gene
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
Yahoo.com and AOL/TW attorneys please note, additions to the above
message by Gene Heskett are:
Copyright 2006 by Maurice Eugene Heskett, all rights reserved.

Re: Linux 2.6.20-rc1

[Alessandro Suardi]

On 12/14/06, Linus Torvalds <torvalds@osdl.org> wrote:
>
> Ok, the two-week merge period is over, and -rc1 is out there.

Still need this libata-sff.c patch:

http://marc.theaimsgroup.com/?l=linux-kernel&m=116343564202844&q=raw

 to have my root device detected, ata_piix probe would otherwise
 fail as described in this thread:

http://www.ussg.iu.edu/hypermail/linux/kernel/0612.0/0690.html

--alessandro

"...when I get it, I _get_ it"

     (Lara Eidemiller)

Re: Linux 2.6.20-rc1

[Steve WIse]

And the patch was reposted here:

http://marc.theaimsgroup.com/?l=linux-kernel&m=116594961106441&w=2


On Thu, 2006-12-14 at 14:59 +0100, Alessandro Suardi wrote:
> On 12/14/06, Linus Torvalds <torvalds@osdl.org> wrote:
> >
> > Ok, the two-week merge period is over, and -rc1 is out there.
> 
> Still need this libata-sff.c patch:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=116343564202844&q=raw
> 
>  to have my root device detected, ata_piix probe would otherwise
>  fail as described in this thread:
> 
> http://www.ussg.iu.edu/hypermail/linux/kernel/0612.0/0690.html
> 
> --alessandro
> 
> "...when I get it, I _get_ it"
> 
>      (Lara Eidemiller)
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Linux 2.6.20-rc1

[Alan]

On Thu, 14 Dec 2006 14:59:23 +0100
"Alessandro Suardi" <alessandro.suardi@gmail.com> wrote:

> On 12/14/06, Linus Torvalds <torvalds@osdl.org> wrote:
> >
> > Ok, the two-week merge period is over, and -rc1 is out there.
> 
> Still need this libata-sff.c patch:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=116343564202844&q=raw
> 
>  to have my root device detected, ata_piix probe would otherwise
>  fail as described in this thread:
> 
> http://www.ussg.iu.edu/hypermail/linux/kernel/0612.0/0690.html
> 

Yep - sorry about not dealing with this yet but I've not had opportunity
to do much but email. I'm grabbing 20-rc1 atm to check there are no other
outstanding bits.

Alan

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

Hi Linus,

`hddtemp' has stopped working on 2.6.20-rc1:

[root] 19:25 [~] hddtemp /dev/sda /dev/sdb /dev/sdc /dev/sdd
/dev/sda: ATA WDC WD2500KS-00M: S.M.A.R.T. not available
/dev/sdb: ATA WDC WD2500KS-00M: S.M.A.R.T. not available
/dev/sdc: ATA Maxtor 6B200M0: S.M.A.R.T. not available
/dev/sdd: ATA Maxtor 6B200M0: S.M.A.R.T. not available

Stracing the binary reveals:

open("/dev/sdd", O_RDONLY|O_NONBLOCK)   = 3
ioctl(3, SCSI_IOCTL_GET_BUS_NUMBER, 0x7fffe8a0636c) = 0
ioctl(3, SG_IO, 0x7fffe8a06020)         = 0
ioctl(3, SG_IO, 0x7fffe8a06040)         = 0
ioctl(3, 0x30d, 0x506b80)               = -1 ENOTTY (Inappropriate ioctl for device)
ioctl(3, SCSI_IOCTL_GET_BUS_NUMBER, 0x7fffe8a06384) = 0
ioctl(3, SG_IO, 0x7fffe8a06240)         = 0
open("/dev/sdc", O_RDONLY|O_NONBLOCK)   = 4
ioctl(4, SCSI_IOCTL_GET_BUS_NUMBER, 0x7fffe8a0636c) = 0
ioctl(4, SG_IO, 0x7fffe8a06020)         = 0
ioctl(4, SG_IO, 0x7fffe8a06040)         = 0
ioctl(4, 0x30d, 0x506b80)               = -1 ENOTTY (Inappropriate ioctl for device)
ioctl(4, SCSI_IOCTL_GET_BUS_NUMBER, 0x7fffe8a06384) = 0
ioctl(4, SG_IO, 0x7fffe8a06240)         = 0
open("/dev/sdb", O_RDONLY|O_NONBLOCK)   = 5
ioctl(5, SCSI_IOCTL_GET_BUS_NUMBER, 0x7fffe8a0636c) = 0
ioctl(5, SG_IO, 0x7fffe8a06020)         = 0
ioctl(5, SG_IO, 0x7fffe8a06040)         = 0
ioctl(5, 0x30d, 0x506b80)               = -1 ENOTTY (Inappropriate ioctl for device)
ioctl(5, SCSI_IOCTL_GET_BUS_NUMBER, 0x7fffe8a06384) = 0
ioctl(5, SG_IO, 0x7fffe8a06240)         = 0
open("/dev/sda", O_RDONLY|O_NONBLOCK)   = 6
ioctl(6, SCSI_IOCTL_GET_BUS_NUMBER, 0x7fffe8a0636c) = 0
ioctl(6, SG_IO, 0x7fffe8a06020)         = 0
ioctl(6, SG_IO, 0x7fffe8a06040)         = 0
ioctl(6, 0x30d, 0x506b80)               = -1 ENOTTY (Inappropriate ioctl for device)
ioctl(6, SCSI_IOCTL_GET_BUS_NUMBER, 0x7fffe8a06384) = 0
ioctl(6, SG_IO, 0x7fffe8a06240)         = 0
ioctl(6, SG_IO, 0x7fffe8a05d20)         = 0

Is there a known workaround for this?

SMART is enabled in the BIOS and it's available in 2.6.19.

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Linus Torvalds]

On Thu, 14 Dec 2006, Alistair John Strachan wrote:
> 
> `hddtemp' has stopped working on 2.6.20-rc1:

Hmm. Can you do the strace on a working kernel too? For example, is it 
that the 0x30d ioctl (which is HDIO_GET_IDENTITY) used to work? If it's a 
SATA device, and you _used_ to use the PATA drivers, some of the old 
IDE-only ioctl's simply don't work when used in native SATA 
configurations.

[ Side note: I consider that to be a mis-feature, but it's not a new 
  regression, it's always been that way: different block subsystems have 
  had their own "private" ioctl spaces.

  We've been moving more and more towards a unified space, and we could 
  probably make scsi_ioctl.c emulate at least _some_ of the HDIO_xxx calls 
  too, and try to support all the block ioctl's on all block devices 
  rather than have some that work only on some certain class of hardware. 

  But we're not there yet, and in the meantime it will actually make a 
  difference whether you use your disks through the kernel SCSI layer 
  (SATA and /dev/sdX) or through the IDE layer (IDE and /dev/hdX) ]

On the other hand, this _sounds_ very much like a bug that should have 
been fixed before 2.6.20-rc1, which affected SG_IO. 

If you can do a "git bisect" on this, that would help a lot.

(Btw, where is "hddtemp" from, anyway? Doesn't seem to be part of the 
standard set of tools I have on any of my systems)

		Linus

Re: Linux 2.6.20-rc1

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Thu, 14 Dec 2006, Alistair John Strachan wrote:
>> `hddtemp' has stopped working on 2.6.20-rc1:
> 
> Hmm. Can you do the strace on a working kernel too? For example, is it 
> that the 0x30d ioctl (which is HDIO_GET_IDENTITY) used to work? If it's a 
> SATA device, and you _used_ to use the PATA drivers, some of the old 
> IDE-only ioctl's simply don't work when used in native SATA 
> configurations.
> 
> [ Side note: I consider that to be a mis-feature, but it's not a new 
>   regression, it's always been that way: different block subsystems have 
>   had their own "private" ioctl spaces.
> 
>   We've been moving more and more towards a unified space, and we could 
>   probably make scsi_ioctl.c emulate at least _some_ of the HDIO_xxx calls 
>   too, and try to support all the block ioctl's on all block devices 
>   rather than have some that work only on some certain class of hardware. 

FWIW, libata generally follows a "implement it, if enough people care 
about it" policy for the old HDIO_xxx ioctls.

There are plenty of HDIO_xxx ioctl should that have died back in the 
days when people using the 'hd' driver rather than the newfangled IDE 
driver.

So this change sorta filters out a lot of those older ioctls.

hddtemp is open source and reasonably well known, so I would certainly 
like to support it, if its reasonable.  For ATA disks, obtaining the 
temperature sometimes requires vendor-specific or firmware-specific 
knowledge.  hddtemp centralizes all that info in a database.

	Jeff

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

On Thursday 14 December 2006 19:57, Linus Torvalds wrote:
> On Thu, 14 Dec 2006, Alistair John Strachan wrote:
> > `hddtemp' has stopped working on 2.6.20-rc1:
>
> Hmm. Can you do the strace on a working kernel too? For example, is it
> that the 0x30d ioctl (which is HDIO_GET_IDENTITY) used to work? If it's a
> SATA device, and you _used_ to use the PATA drivers, some of the old
> IDE-only ioctl's simply don't work when used in native SATA
> configurations.

I've always been using sata_nv and libata. All the drives in question are SATA 
devices, no configuration change other than this kernel has taken place.

Indeed, the configs are very similar. Find the configs, straces on both 
kernels, and the hddtemp binary (AMD64, I'm afraid) here:

http://devzero.co.uk/~alistair/2.6.20-rc1-hddtemp/

> [ Side note: I consider that to be a mis-feature, but it's not a new
>   regression, it's always been that way: different block subsystems have
>   had their own "private" ioctl spaces.
>
>   We've been moving more and more towards a unified space, and we could
>   probably make scsi_ioctl.c emulate at least _some_ of the HDIO_xxx calls
>   too, and try to support all the block ioctl's on all block devices
>   rather than have some that work only on some certain class of hardware.
>
>   But we're not there yet, and in the meantime it will actually make a
>   difference whether you use your disks through the kernel SCSI layer
>   (SATA and /dev/sdX) or through the IDE layer (IDE and /dev/hdX) ]
>
> On the other hand, this _sounds_ very much like a bug that should have
> been fixed before 2.6.20-rc1, which affected SG_IO.
>
> If you can do a "git bisect" on this, that would help a lot.

I'll do that if nobody comes up with anything obvious.

> (Btw, where is "hddtemp" from, anyway? Doesn't seem to be part of the
> standard set of tools I have on any of my systems)

http://www.guzu.net/linux/hddtemp.php

I run this in monitor mode, and then use the gkrellm plugin to keep an eye on 
the disk temperatures. It's worked well for years, until now.

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Thu, Dec 14 2006, Alistair John Strachan wrote:
> On Thursday 14 December 2006 19:57, Linus Torvalds wrote:
> > On Thu, 14 Dec 2006, Alistair John Strachan wrote:
> > > `hddtemp' has stopped working on 2.6.20-rc1:
> >
> > Hmm. Can you do the strace on a working kernel too? For example, is it
> > that the 0x30d ioctl (which is HDIO_GET_IDENTITY) used to work? If it's a
> > SATA device, and you _used_ to use the PATA drivers, some of the old
> > IDE-only ioctl's simply don't work when used in native SATA
> > configurations.
> 
> I've always been using sata_nv and libata. All the drives in question are SATA 
> devices, no configuration change other than this kernel has taken place.
> 
> Indeed, the configs are very similar. Find the configs, straces on both 
> kernels, and the hddtemp binary (AMD64, I'm afraid) here:
> 
> http://devzero.co.uk/~alistair/2.6.20-rc1-hddtemp/

Looking at the strace, it would _seem_ that an SG_IO failure could very
well be the reason for the diverged paths. And that would indicate
another bug in that area, outside of what we already fixed for
2.6.20-rc1.

Is the hddtemp source not available?

> > If you can do a "git bisect" on this, that would help a lot.
> 
> I'll do that if nobody comes up with anything obvious.

If you can just test 2.6.19-git1, then we'll know if it's the SG_IO
patch again.

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Jeff Garzik]

Jens Axboe wrote:
> Is the hddtemp source not available?


http://www.guzu.net/linux/hddtemp.php says 
http://www.guzu.net/files/hddtemp-0.3-beta15.tar.bz2

	Jeff

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Thu, Dec 14 2006, Jeff Garzik wrote:
> Jens Axboe wrote:
> >Is the hddtemp source not available?
> 
> 
> http://www.guzu.net/linux/hddtemp.php says 
> http://www.guzu.net/files/hddtemp-0.3-beta15.tar.bz2

Thanks! I'll await the 2.6.19-git1 test to see how to proceede.

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Thu, Dec 14 2006, Jens Axboe wrote:
> > I'll do that if nobody comes up with anything obvious.
> 
> If you can just test 2.6.19-git1, then we'll know if it's the SG_IO
> patch again.

Actually, you should test 2.6.19-git1 with this patch applied as well.

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Mon, 11 Dec 2006 09:01:34 +0000 (+0100)
Subject: [PATCH] fix SG_IO bio leak
X-Git-Url: http://git.home.kernel.dk/?p=linux-2.6-block.git;a=commitdiff;h=77d172ce2719b5ad2dc0637452c8871d9cba344c

[PATCH] fix SG_IO bio leak

This patch fixes bio leaks in SG_IO. rq->bio can be changed after io
completion, so we need to reset rq->bio before calling blk_rq_unmap_user()

http://marc.theaimsgroup.com/?l=linux-kernel&m=116570666807983&w=2

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---

--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -228,6 +228,7 @@ static int sg_io(struct file *file, requ
 	struct request *rq;
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	unsigned char cmd[BLK_MAX_CDB];
+	struct bio *bio;
 
 	if (hdr->interface_id != 'S')
 		return -EINVAL;
@@ -308,6 +309,7 @@ static int sg_io(struct file *file, requ
 	if (ret)
 		goto out;
 
+	bio = rq->bio;
 	rq->retries = 0;
 
 	start_time = jiffies;
@@ -338,6 +340,7 @@ static int sg_io(struct file *file, requ
 			hdr->sb_len_wr = len;
 	}
 
+	rq->bio = bio;
 	if (blk_rq_unmap_user(rq))
 		ret = -EFAULT;
 

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

Hi Jens,

On Thursday 14 December 2006 20:48, Jens Axboe wrote:
> On Thu, Dec 14 2006, Jens Axboe wrote:
> > > I'll do that if nobody comes up with anything obvious.
> >
> > If you can just test 2.6.19-git1, then we'll know if it's the SG_IO
> > patch again.
>
> Actually, you should test 2.6.19-git1 with this patch applied as well.

2.6.19-git1 with FUJITA Tomonori's bio-leak fix doesn't break, and hddtemp 
continues to work fine:

[root] 21:10 [~] hddtemp /dev/sda /dev/sdb /dev/sdc /dev/sdd
/dev/sda: WDC WD2500KS-00MJB0: 29°C
/dev/sdb: WDC WD2500KS-00MJB0: 27°C
/dev/sdc: Maxtor 6B200M0: 28°C
/dev/sdd: Maxtor 6B200M0: 26°C

I've added the strace results to the URL previously posted, with the config.

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Thu, Dec 14 2006, Alistair John Strachan wrote:
> Hi Jens,
> 
> On Thursday 14 December 2006 20:48, Jens Axboe wrote:
> > On Thu, Dec 14 2006, Jens Axboe wrote:
> > > > I'll do that if nobody comes up with anything obvious.
> > >
> > > If you can just test 2.6.19-git1, then we'll know if it's the SG_IO
> > > patch again.
> >
> > Actually, you should test 2.6.19-git1 with this patch applied as well.
> 
> 2.6.19-git1 with FUJITA Tomonori's bio-leak fix doesn't break, and hddtemp 
> continues to work fine:
> 
> [root] 21:10 [~] hddtemp /dev/sda /dev/sdb /dev/sdc /dev/sdd
> /dev/sda: WDC WD2500KS-00MJB0: 29°C
> /dev/sdb: WDC WD2500KS-00MJB0: 27°C
> /dev/sdc: Maxtor 6B200M0: 28°C
> /dev/sdd: Maxtor 6B200M0: 26°C
> 
> I've added the strace results to the URL previously posted, with the config.

Then it is likely the sata updates, SG_IO is off the hook.

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

On Thursday 14 December 2006 21:20, Jens Axboe wrote:
> On Thu, Dec 14 2006, Alistair John Strachan wrote:
> > Hi Jens,
> >
> > On Thursday 14 December 2006 20:48, Jens Axboe wrote:
> > > On Thu, Dec 14 2006, Jens Axboe wrote:
> > > > > I'll do that if nobody comes up with anything obvious.
> > > >
> > > > If you can just test 2.6.19-git1, then we'll know if it's the SG_IO
> > > > patch again.
> > >
> > > Actually, you should test 2.6.19-git1 with this patch applied as well.
> >
> > 2.6.19-git1 with FUJITA Tomonori's bio-leak fix doesn't break, and
> > hddtemp continues to work fine:
> >
> > [root] 21:10 [~] hddtemp /dev/sda /dev/sdb /dev/sdc /dev/sdd
> > /dev/sda: WDC WD2500KS-00MJB0: 29°C
> > /dev/sdb: WDC WD2500KS-00MJB0: 27°C
> > /dev/sdc: Maxtor 6B200M0: 28°C
> > /dev/sdd: Maxtor 6B200M0: 26°C
> >
> > I've added the strace results to the URL previously posted, with the
> > config.
>
> Then it is likely the sata updates, SG_IO is off the hook.

I bisected all the way down to 0e75f9063f5c55fb0b0b546a7c356f8ec186825e, which 
git reckons is the culprit. I wasn't able to revert this commit to test, 
because it has conflicts.

Any ideas?

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

On Friday 15 December 2006 00:48, Alistair John Strachan wrote:
> On Thursday 14 December 2006 21:20, Jens Axboe wrote:
> > On Thu, Dec 14 2006, Alistair John Strachan wrote:
> > > Hi Jens,
> > >
> > > On Thursday 14 December 2006 20:48, Jens Axboe wrote:
> > > > On Thu, Dec 14 2006, Jens Axboe wrote:
> > > > > > I'll do that if nobody comes up with anything obvious.
> > > > >
> > > > > If you can just test 2.6.19-git1, then we'll know if it's the SG_IO
> > > > > patch again.
> > > >
> > > > Actually, you should test 2.6.19-git1 with this patch applied as
> > > > well.
> > >
> > > 2.6.19-git1 with FUJITA Tomonori's bio-leak fix doesn't break, and
> > > hddtemp continues to work fine:
> > >
> > > [root] 21:10 [~] hddtemp /dev/sda /dev/sdb /dev/sdc /dev/sdd
> > > /dev/sda: WDC WD2500KS-00MJB0: 29°C
> > > /dev/sdb: WDC WD2500KS-00MJB0: 27°C
> > > /dev/sdc: Maxtor 6B200M0: 28°C
> > > /dev/sdd: Maxtor 6B200M0: 26°C
> > >
> > > I've added the strace results to the URL previously posted, with the
> > > config.
> >
> > Then it is likely the sata updates, SG_IO is off the hook.
>
> I bisected all the way down to 0e75f9063f5c55fb0b0b546a7c356f8ec186825e,
> which git reckons is the culprit. I wasn't able to revert this commit to
> test, because it has conflicts.

Whatever the change is, it's subtle. I don't see the problem in git1+patch, 
but I know this kernel _includes_ this changeset.

In total isolation, v2.6.19..0e75f9063f5c55fb0b0b546a7c356f8ec186825e it 
breaks. Reverting just 0e75f9063f5c55fb0b0b546a7c356f8ec186825e, it works 
again.

So I think this is the source, but I can't explain why it "goes away" before 
git1 and "comes back" before 2.6.20-rc1.

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Linus Torvalds]

On Fri, 15 Dec 2006, Alistair John Strachan wrote:
> 
> In total isolation, v2.6.19..0e75f9063f5c55fb0b0b546a7c356f8ec186825e it 
> breaks. Reverting just 0e75f9063f5c55fb0b0b546a7c356f8ec186825e, it works 
> again.
> 
> So I think this is the source, but I can't explain why it "goes away" before 
> git1 and "comes back" before 2.6.20-rc1.

Can you see if the kernel state at commit 77d172ce ("[PATCH] fix SG_IO bio 
leak") is good? Ie just do something like

	git checkout -b test-branch 77d172ce

and compile and test that?

That commit _should_ be the one that fixed whatever problems that commit 
0e75f906 introduced. It *did* fix it for other - somewhat similar - 
situations.

That said: Jens - I think 0e75f906 was a mistake. "blk_rq_unmap()" really 
should be passed the "struct bio", not the "struct request *". Right now 
it does something _really_ strange with requests with linked bio's, and I 
don't think your and FUJITA's "leak fix" really works. What happens when 
the bio was a linked list on the request, and you put the old _head_ on 
the request with "rq->bio = bio"? What happens to the other parts of it?

IOW, I think this is broken. I think we should revert 0e75f906. Or at 
least you should explain to me why it's not broken, and why clearly people 
(eg Alistair) still see problems with it?

		Linus

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

On Saturday 16 December 2006 21:36, Linus Torvalds wrote:
> On Fri, 15 Dec 2006, Alistair John Strachan wrote:
> > In total isolation, v2.6.19..0e75f9063f5c55fb0b0b546a7c356f8ec186825e it
> > breaks. Reverting just 0e75f9063f5c55fb0b0b546a7c356f8ec186825e, it works
> > again.
> >
> > So I think this is the source, but I can't explain why it "goes away"
> > before git1 and "comes back" before 2.6.20-rc1.
>
> Can you see if the kernel state at commit 77d172ce ("[PATCH] fix SG_IO bio
> leak") is good? Ie just do something like
>
> 	git checkout -b test-branch 77d172ce
>
> and compile and test that?
>
> That commit _should_ be the one that fixed whatever problems that commit
> 0e75f906 introduced. It *did* fix it for other - somewhat similar -
> situations.
>
> That said: Jens - I think 0e75f906 was a mistake. "blk_rq_unmap()" really
> should be passed the "struct bio", not the "struct request *". Right now
> it does something _really_ strange with requests with linked bio's, and I
> don't think your and FUJITA's "leak fix" really works. What happens when
> the bio was a linked list on the request, and you put the old _head_ on
> the request with "rq->bio = bio"? What happens to the other parts of it?
>
> IOW, I think this is broken. I think we should revert 0e75f906. Or at
> least you should explain to me why it's not broken, and why clearly people
> (eg Alistair) still see problems with it?

It could simply be that bisect isn't working here because it's actually broken
by two separate patches. Out of bad luck, I've ended up singling out the one
that already has a "fix", and the "real bug" hasn't been found.

I guess I should repeat the bisection, and when the bio-fix isn't applied,
manually apply it? Is there some magical Git way to do this?

Here's the bisection log, for what it's worth;

[alistair] 22:27 [~/linux-git] git bisect log
git-bisect start
# bad: [cc016448b0bf0764928275d034e367753bde8162] Linux v2.6.20-rc1
git-bisect bad cc016448b0bf0764928275d034e367753bde8162
# good: [c3fe6924620fd733ffe8bc8a9da1e9cde08402b3] Linux 2.6.19
git-bisect good c3fe6924620fd733ffe8bc8a9da1e9cde08402b3
# bad: [b2c03941b50944a268ee4d5823872f220809a3ba] IPMI: Allow hot system interface remove
git-bisect bad b2c03941b50944a268ee4d5823872f220809a3ba
# bad: [29b08d2bae854f66d3cfd5f57aaf2e7c2c7fce32] [S390] pfault code cleanup.
git-bisect bad 29b08d2bae854f66d3cfd5f57aaf2e7c2c7fce32
# bad: [0e11c91e1e912bc4db5b71607d149e7e9a77e756] [AF_PACKET]: annotate
git-bisect bad 0e11c91e1e912bc4db5b71607d149e7e9a77e756
# bad: [5f56bbdf1e35d41b4b3d4c92bdb3e70c63877e4d] Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/roland/infiniband
git-bisect bad 5f56bbdf1e35d41b4b3d4c92bdb3e70c63877e4d
# good: [94fcda1f8ab5e0cacc381c5ca1cc9aa6ad523576] usbcore: remove unused argument in autosuspend
git-bisect good 94fcda1f8ab5e0cacc381c5ca1cc9aa6ad523576
# bad: [4549df891a31b9a05b7d183106c09049b79327be] Merge master.kernel.org:/pub/scm/linux/kernel/git/gregkh/driver-2.6
git-bisect bad 4549df891a31b9a05b7d183106c09049b79327be
# good: [5ab699810d46011ad2195c5916f3cbc684bfe3ee] driver core: Introduce device_find_child().
git-bisect good 5ab699810d46011ad2195c5916f3cbc684bfe3ee
# good: [6b44d4e69c6144d0df71ab47ec90d2009237d48f] Fix typos in drivers/isdn/hisax/isdnl2.c
git-bisect good 6b44d4e69c6144d0df71ab47ec90d2009237d48f
# bad: [6b8cc71ab2619a776b02869fd733ac1ead3db4e8] Merge git://git.kernel.org/pub/scm/linux/kernel/git/sfrench/cifs-2.6
git-bisect bad 6b8cc71ab2619a776b02869fd733ac1ead3db4e8
# bad: [bb37b94c68e7b37eecea8576039ae9396ca07839] [BLOCK] Cleanup unused variable passing
git-bisect bad bb37b94c68e7b37eecea8576039ae9396ca07839
# good: [ad2d7225709b11da47e092634cbdf0591829ae9c] block: kill length alignment test in bio_map_user()
git-bisect good ad2d7225709b11da47e092634cbdf0591829ae9c
# bad: [0e75f9063f5c55fb0b0b546a7c356f8ec186825e] block: support larger block pc requests
git-bisect bad 0e75f9063f5c55fb0b0b546a7c356f8ec186825e

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Jeff Garzik]

Alistair John Strachan wrote:
> It could simply be that bisect isn't working here because it's actually broken
> by two separate patches. Out of bad luck, I've ended up singling out the one
> that already has a "fix", and the "real bug" hasn't been found.
> 
> I guess I should repeat the bisection, and when the bio-fix isn't applied,
> manually apply it? Is there some magical Git way to do this?
> 
> Here's the bisection log, for what it's worth;


This may be totally subjective on my part, but if I get stuck (and I 
have the patience) I sometimes like to look at the log and try to pick 
arbitrary 'good' points, to stir the pot a bit.

	Jeff

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

On Saturday 16 December 2006 21:36, Linus Torvalds wrote:
> On Fri, 15 Dec 2006, Alistair John Strachan wrote:
> > In total isolation, v2.6.19..0e75f9063f5c55fb0b0b546a7c356f8ec186825e it
> > breaks. Reverting just 0e75f9063f5c55fb0b0b546a7c356f8ec186825e, it works
> > again.
> >
> > So I think this is the source, but I can't explain why it "goes away"
> > before git1 and "comes back" before 2.6.20-rc1.
>
> Can you see if the kernel state at commit 77d172ce ("[PATCH] fix SG_IO bio
> leak") is good? Ie just do something like
>
> 	git checkout -b test-branch 77d172ce
>
> and compile and test that?

As predicted, it still doesn't work.

[alistair] 22:59 [~/linux-git] git-status
# On branch refs/heads/test-branch
nothing to commit

[root] 22:59 [~] hddtemp /dev/sda
/dev/sda: ATA WDC WD2500KS-00M: S.M.A.R.T. not available

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Sat, Dec 16 2006, Linus Torvalds wrote:
> That said: Jens - I think 0e75f906 was a mistake. "blk_rq_unmap()" really 
> should be passed the "struct bio", not the "struct request *". Right now 
> it does something _really_ strange with requests with linked bio's, and I 
> don't think your and FUJITA's "leak fix" really works. What happens when 
> the bio was a linked list on the request, and you put the old _head_ on 
> the request with "rq->bio = bio"? What happens to the other parts of it?

I agree it's fishy and I did think about it. The design isn't exactly
the prettiest, but it should be safe. The reason is that we don't
actually unlink the individual bio from the list, even if we may set
rq->bio to point somewhere further into the list. So as long as the bio
is valid, the bi_next field is still valid as well. We need a reference
on the bio to perform the unmap and blk_rq_unmap_user() drops this
reference on its own, so the bio must be valid.

Taking a rq pointer when we really want a bio is nasty, though. I'll
chance that at least.

> IOW, I think this is broken. I think we should revert 0e75f906. Or at 
> least you should explain to me why it's not broken, and why clearly people 
> (eg Alistair) still see problems with it?

I'm not so sure it's that patch, the same problem seemed to exist for
some people prior to 2.6.20.

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Mon, Dec 18 2006, Jens Axboe wrote:
> On Sat, Dec 16 2006, Linus Torvalds wrote:
> > That said: Jens - I think 0e75f906 was a mistake. "blk_rq_unmap()" really 
> > should be passed the "struct bio", not the "struct request *". Right now 
> > it does something _really_ strange with requests with linked bio's, and I 
> > don't think your and FUJITA's "leak fix" really works. What happens when 
> > the bio was a linked list on the request, and you put the old _head_ on 
> > the request with "rq->bio = bio"? What happens to the other parts of it?
> 
> I agree it's fishy and I did think about it. The design isn't exactly
> the prettiest, but it should be safe. The reason is that we don't
> actually unlink the individual bio from the list, even if we may set
> rq->bio to point somewhere further into the list. So as long as the bio
> is valid, the bi_next field is still valid as well. We need a reference
> on the bio to perform the unmap and blk_rq_unmap_user() drops this
> reference on its own, so the bio must be valid.
> 
> Taking a rq pointer when we really want a bio is nasty, though. I'll
> chance that at least.

Something like this. One alternative I did originally consider was to
add a rq->cbio (for lack of a better name) that points to the original
bio and isn't cleared on io completion, just to have the original bio
location stored. That makes the API symmetric and doesn't have any
hidden requirements, at the cost of an extra pointer in struct request.
So I think the included is the better patch, since it's still clear what
needs to be done and it doesn't add extra members to struct request.

-----

The blk_rq_unmap_user() API is not very nice. It expects the caller to
know that rq->bio has to be reset to the original bio, and it will
silently do nothing if that is not done. Instead make it explicit that
we need to pass in the first bio, by expecting a bio argument.

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index fb40575..51c828e 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -2323,6 +2323,7 @@ int blk_rq_map_user(request_queue_t *q, struct request *rq, void __user *ubuf,
 		    unsigned long len)
 {
 	unsigned long bytes_read = 0;
+	struct bio *bio = NULL;
 	int ret;
 
 	if (len > (q->max_hw_sectors << 9))
@@ -2349,6 +2350,8 @@ int blk_rq_map_user(request_queue_t *q, struct request *rq, void __user *ubuf,
 		ret = __blk_rq_map_user(q, rq, ubuf, map_len);
 		if (ret < 0)
 			goto unmap_rq;
+		if (!bio)
+			bio = rq->bio;
 		bytes_read += ret;
 		ubuf += ret;
 	}
@@ -2356,7 +2359,7 @@ int blk_rq_map_user(request_queue_t *q, struct request *rq, void __user *ubuf,
 	rq->buffer = rq->data = NULL;
 	return 0;
 unmap_rq:
-	blk_rq_unmap_user(rq);
+	blk_rq_unmap_user(bio);
 	return ret;
 }
 
@@ -2413,26 +2416,28 @@ EXPORT_SYMBOL(blk_rq_map_user_iov);
 
 /**
  * blk_rq_unmap_user - unmap a request with user data
- * @rq:		rq to be unmapped
+ * @bio:		start of bio list
  *
  * Description:
- *    Unmap a rq previously mapped by blk_rq_map_user().
- *    rq->bio must be set to the original head of the request.
+ *    Unmap a rq previously mapped by blk_rq_map_user(). The caller must
+ *    supply the original rq->bio from the blk_rq_map_user() return, since
+ *    the io completion may have changed rq->bio.
  */
-int blk_rq_unmap_user(struct request *rq)
+int blk_rq_unmap_user(struct bio *bio)
 {
-	struct bio *bio, *mapped_bio;
+	struct bio *mapped_bio;
 
-	while ((bio = rq->bio)) {
-		if (bio_flagged(bio, BIO_BOUNCED))
+	while (bio) {
+		mapped_bio = bio;
+		if (unlikely(bio_flagged(bio, BIO_BOUNCED)))
 			mapped_bio = bio->bi_private;
-		else
-			mapped_bio = bio;
 
 		__blk_rq_unmap_user(mapped_bio);
-		rq->bio = bio->bi_next;
-		bio_put(bio);
+		mapped_bio = bio;
+		bio = bio->bi_next;
+		bio_put(mapped_bio);
 	}
+
 	return 0;
 }
 
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index f322b6a..2528a0c 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -333,8 +333,7 @@ static int sg_io(struct file *file, request_queue_t *q,
 			hdr->sb_len_wr = len;
 	}
 
-	rq->bio = bio;
-	if (blk_rq_unmap_user(rq))
+	if (blk_rq_unmap_user(bio))
 		ret = -EFAULT;
 
 	/* may not have succeeded, but output values written to control
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index e4a2f8f..66d028d 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2139,8 +2139,7 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
 			cdi->last_sense = s->sense_key;
 		}
 
-		rq->bio = bio;
-		if (blk_rq_unmap_user(rq))
+		if (blk_rq_unmap_user(bio))
 			ret = -EFAULT;
 
 		if (ret)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 0a801cc..d93f8ea 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -710,7 +710,7 @@ extern void __blk_stop_queue(request_queue_t *q);
 extern void blk_run_queue(request_queue_t *);
 extern void blk_start_queueing(request_queue_t *);
 extern int blk_rq_map_user(request_queue_t *, struct request *, void __user *, unsigned long);
-extern int blk_rq_unmap_user(struct request *);
+extern int blk_rq_unmap_user(struct bio *);
 extern int blk_rq_map_kern(request_queue_t *, struct request *, void *, unsigned int, gfp_t);
 extern int blk_rq_map_user_iov(request_queue_t *, struct request *,
 			       struct sg_iovec *, int, unsigned int);

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Jeff Garzik]

Alistair John Strachan wrote:
> Hi Jens,
> 
> On Thursday 14 December 2006 20:48, Jens Axboe wrote:
>> On Thu, Dec 14 2006, Jens Axboe wrote:
>>>> I'll do that if nobody comes up with anything obvious.
>>> If you can just test 2.6.19-git1, then we'll know if it's the SG_IO
>>> patch again.
>> Actually, you should test 2.6.19-git1 with this patch applied as well.
> 
> 2.6.19-git1 with FUJITA Tomonori's bio-leak fix doesn't break, and hddtemp 
> continues to work fine:
> 
> [root] 21:10 [~] hddtemp /dev/sda /dev/sdb /dev/sdc /dev/sdd
> /dev/sda: WDC WD2500KS-00MJB0: 29°C
> /dev/sdb: WDC WD2500KS-00MJB0: 27°C
> /dev/sdc: Maxtor 6B200M0: 28°C
> /dev/sdd: Maxtor 6B200M0: 26°C

So can you bisect and see which patch broke things?

I do wonder if its the update to sata_nv ADMA.

	Jeff

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

On Thursday 14 December 2006 21:33, Jeff Garzik wrote:
> Alistair John Strachan wrote:
> > Hi Jens,
> >
> > On Thursday 14 December 2006 20:48, Jens Axboe wrote:
> >> On Thu, Dec 14 2006, Jens Axboe wrote:
> >>>> I'll do that if nobody comes up with anything obvious.
> >>>
> >>> If you can just test 2.6.19-git1, then we'll know if it's the SG_IO
> >>> patch again.
> >>
> >> Actually, you should test 2.6.19-git1 with this patch applied as well.
> >
> > 2.6.19-git1 with FUJITA Tomonori's bio-leak fix doesn't break, and
> > hddtemp continues to work fine:
> >
> > [root] 21:10 [~] hddtemp /dev/sda /dev/sdb /dev/sdc /dev/sdd
> > /dev/sda: WDC WD2500KS-00MJB0: 29°C
> > /dev/sdb: WDC WD2500KS-00MJB0: 27°C
> > /dev/sdc: Maxtor 6B200M0: 28°C
> > /dev/sdd: Maxtor 6B200M0: 26°C
>
> So can you bisect and see which patch broke things?
>
> I do wonder if its the update to sata_nv ADMA.

Before I proceed with the horrors of an -rc1 bisection, could somebody send me 
the ADMA patches so I can eliminate those first?

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Jeff Garzik]

Alistair John Strachan wrote:
> Before I proceed with the horrors of an -rc1 bisection, could somebody send me 
> the ADMA patches so I can eliminate those first?

Run

	git-whatchanged drivers/ata/sata_nv.c

and that will give you a list of recent changes.  To obtain the "diff 
-u" patch for a single commit, run

	git-diff-tree -p $SHA_HASH > /tmp/patch

Regards,

	Jeff

Re: Linux 2.6.20-rc1

[Alistair John Strachan]

On Thursday 14 December 2006 21:50, Jeff Garzik wrote:
> Alistair John Strachan wrote:
> > Before I proceed with the horrors of an -rc1 bisection, could somebody
> > send me the ADMA patches so I can eliminate those first?
>
> Run
>
> 	git-whatchanged drivers/ata/sata_nv.c
>
> and that will give you a list of recent changes.  To obtain the "diff
> -u" patch for a single commit, run
>
> 	git-diff-tree -p $SHA_HASH > /tmp/patch

I used a variation on this:

	git-whatchanged -p v2.6.19.. drivers/ata/sata_nv.c >sata_nv

Reverted it (against v2.6.20-rc1), compiled that kernel, no dice.

[root] 22:32 [~] hddtemp /dev/sda
/dev/sda: ATA WDC WD2500KS-00M: S.M.A.R.T. not available

I'll start the bisection.

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Thu, Dec 14 2006, Alistair John Strachan wrote:
> On Thursday 14 December 2006 21:50, Jeff Garzik wrote:
> > Alistair John Strachan wrote:
> > > Before I proceed with the horrors of an -rc1 bisection, could somebody
> > > send me the ADMA patches so I can eliminate those first?
> >
> > Run
> >
> > 	git-whatchanged drivers/ata/sata_nv.c
> >
> > and that will give you a list of recent changes.  To obtain the "diff
> > -u" patch for a single commit, run
> >
> > 	git-diff-tree -p $SHA_HASH > /tmp/patch
> 
> I used a variation on this:
> 
> 	git-whatchanged -p v2.6.19.. drivers/ata/sata_nv.c >sata_nv
> 
> Reverted it (against v2.6.20-rc1), compiled that kernel, no dice.
> 
> [root] 22:32 [~] hddtemp /dev/sda
> /dev/sda: ATA WDC WD2500KS-00M: S.M.A.R.T. not available
> 
> I'll start the bisection.

Just noticed that most of the mails I wrote on this thread were
apparently without linux-kernel cc'ed (dunno who removed the cc). So
I'll write a small summary - the problem is that hddtemp includes some
fragile code to check the sense info, and this commit:

http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=f38621b3109068adc8430bc2d170ccea59df4261

broke it. hddtemp expects 14, but it now sees 12. IMHO hddtemp is buggy
and should be fixed, the best option is simply to kill the sense checks
as I think they have little (if any) value. Patch below for that.

So the problem was never the SG_IO changes, the fact that somebody
noticed the same thing in bugzilla for a 2.6.19-rc6-mm kernel backs that
up.

--- hddtemp-0.3-beta15/src/satacmds.c~	2006-12-19 12:01:10.000000000 +0100
+++ hddtemp-0.3-beta15/src/satacmds.c	2006-12-19 12:01:27.000000000 +0100
@@ -54,7 +54,6 @@
   unsigned char cdb[16];
   unsigned char sense[32];
   int dxfer_direction;
-  int ret;
   
   memset(cdb, 0, sizeof(cdb));
   cdb[0] = ATA_16;
@@ -78,13 +77,7 @@
     cdb[6] = cmd[1];
   cdb[14] = cmd[0];
 
-  ret = scsi_SG_IO(device, cdb, sizeof(cdb), buffer, cmd[3] * 512, sense, sizeof(sense), dxfer_direction);
- 
-  /* Verify SATA magics */
-  if (sense[0] != 0x72 || sense[7] != 0x0e || sense[9] != 0x0e || sense[10] != 0x00)
-    return 1;		  
-  else 
-    return ret;
+  return scsi_SG_IO(device, cdb, sizeof(cdb), buffer, cmd[3] * 512, sense, sizeof(sense), dxfer_direction);
 }
 
 void sata_fixstring(unsigned char *s, int bytecount)

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Robert Hancock]

Jens Axboe wrote:
> Just noticed that most of the mails I wrote on this thread were
> apparently without linux-kernel cc'ed (dunno who removed the cc). So
> I'll write a small summary - the problem is that hddtemp includes some
> fragile code to check the sense info, and this commit:
> 
> http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=f38621b3109068adc8430bc2d170ccea59df4261
> 
> broke it. hddtemp expects 14, but it now sees 12. IMHO hddtemp is buggy
> and should be fixed, the best option is simply to kill the sense checks
> as I think they have little (if any) value. Patch below for that.
> 
> So the problem was never the SG_IO changes, the fact that somebody
> noticed the same thing in bugzilla for a 2.6.19-rc6-mm kernel backs that
> up.

 From what I've seen it appears that smartctl has the same problem, it 
was also reporting the device didn't support SMART..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Tue, Dec 19 2006, Robert Hancock wrote:
> Jens Axboe wrote:
> >Just noticed that most of the mails I wrote on this thread were
> >apparently without linux-kernel cc'ed (dunno who removed the cc). So
> >I'll write a small summary - the problem is that hddtemp includes some
> >fragile code to check the sense info, and this commit:
> >
> >http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=f38621b3109068adc8430bc2d170ccea59df4261
> >
> >broke it. hddtemp expects 14, but it now sees 12. IMHO hddtemp is buggy
> >and should be fixed, the best option is simply to kill the sense checks
> >as I think they have little (if any) value. Patch below for that.
> >
> >So the problem was never the SG_IO changes, the fact that somebody
> >noticed the same thing in bugzilla for a 2.6.19-rc6-mm kernel backs that
> >up.
> 
> From what I've seen it appears that smartctl has the same problem, it 
> was also reporting the device didn't support SMART..

Can you check whether reverting the above commit makes SMART work again?

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Jens Axboe]

On Tue, Dec 19 2006, Jens Axboe wrote:
> On Tue, Dec 19 2006, Robert Hancock wrote:
> > Jens Axboe wrote:
> > >Just noticed that most of the mails I wrote on this thread were
> > >apparently without linux-kernel cc'ed (dunno who removed the cc). So
> > >I'll write a small summary - the problem is that hddtemp includes some
> > >fragile code to check the sense info, and this commit:
> > >
> > >http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=f38621b3109068adc8430bc2d170ccea59df4261
> > >
> > >broke it. hddtemp expects 14, but it now sees 12. IMHO hddtemp is buggy
> > >and should be fixed, the best option is simply to kill the sense checks
> > >as I think they have little (if any) value. Patch below for that.
> > >
> > >So the problem was never the SG_IO changes, the fact that somebody
> > >noticed the same thing in bugzilla for a 2.6.19-rc6-mm kernel backs that
> > >up.
> > 
> > From what I've seen it appears that smartctl has the same problem, it 
> > was also reporting the device didn't support SMART..
> 
> Can you check whether reverting the above commit makes SMART work again?

smartctl fine for me, with and without the patch.

-- 
Jens Axboe

Re: Linux 2.6.20-rc1

[Linus Torvalds]

On Tue, 19 Dec 2006, Robert Hancock wrote:
> 
> From what I've seen it appears that smartctl has the same problem, it was also
> reporting the device didn't support SMART..

No, there were actually two different problems, and to confuse people, 
they had the same _symptoms_.

Commit 0e75f9063f5c55fb0b0b546a7c356f8ec186825e introduced a bug where 
SG_IO wouldn't copy the data back to user space correctly, which was why 
you got various tools like "dvd+rw-mediainfo" (and probably smartctl too) 
breaking.

That was also probably why bisection didn't pick out the right commit for 
the _other_ bug: it was effectively masked by the same problem, so the 
fact that commit f38621b3109068adc8430bc2d170ccea59df4261 fixed the sense 
info details and broke hddtemp was not visible as a bug, because the sense 
info was _more_ corrupted by the other bug, and thus "git bisect" walked 
back to where the _first_ bug was introduced, rather than the second one.

Anyway, sounds like hddtemp was just buggy. 

		Linus

Re: Linux 2.6.20-rc1

[Jeff Garzik]

Alistair John Strachan wrote:
> Before I proceed with the horrors of an -rc1 bisection, could somebody send me 
> the ADMA patches so I can eliminate those first?


BTW a bisection need not be blindly horrific...  You can look at the 
commit ids from git-whatchanged output mentioned in the previous email, 
and make educated guesses about what might be a good or bad change.

	Jeff

Re: Linux 2.6.20-rc1

[Nicolas Mailhot]

Alistair John Strachan <s0348365 <at> sms.ed.ac.uk> writes:

> `hddtemp' has stopped working on 2.6.20-rc1:

→ http://bugzilla.kernel.org/show_bug.cgi?id=7581

Re: Linux 2.6.20-rc1

[Jeff Garzik]

Nicolas Mailhot wrote:
> Alistair John Strachan <s0348365 <at> sms.ed.ac.uk> writes:
> 
>> `hddtemp' has stopped working on 2.6.20-rc1:
> 
> → http://bugzilla.kernel.org/show_bug.cgi?id=7581

I'm not sure I quite follow your bug report.  Are you saying that the 
patch you attached causes the problem?

	Jeff

Re: Linux 2.6.20-rc1

[Nicolas Mailhot]

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

Le jeudi 14 décembre 2006 à 18:22 -0500, Jeff Garzik a écrit :
> Nicolas Mailhot wrote:
> > Alistair John Strachan <s0348365 <at> sms.ed.ac.uk> writes:
> > 
> >> `hddtemp' has stopped working on 2.6.20-rc1:
> > 
> > → http://bugzilla.kernel.org/show_bug.cgi?id=7581
> 
> I'm not sure I quite follow your bug report.  Are you saying that the 
> patch you attached causes the problem?

No, I'm saying the hddtemp breakage people report against 2.6.20-rc1
also occurred between 2.6.19-rc5-mm2 and 2.6.19-rc6-mm1, as reported in
this bug.

Also this system sports two different (S)ATA controllers (SiI 3114 and
CK804), disks on both stopped reporting temp at the same time so the
change is not chipset-specific

-- 
Nicolas Mailhot

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Linux 2.6.20-rc1

[Bill Davidsen]

Linus Torvalds wrote:
> Ok, the two-week merge period is over, and -rc1 is out there.
> 
> I'm _really_ hoping that we can keep the 2.6.20 release calmer and without 
> any of the dragging-out-due-to-core-changes that we've had lately. We 
> didn't actually merge any really core changes here, with the biggest 
> conceptual one being the "work_struct" split into regular work and 
> "delayed" work, so I'm hoping we can really end up with an easy 2.6.20 
> release.
> 
> Some of the commits there are pretty big patches, but more than a couple 
> of them are due to fairly straightforward search-and-replace things (like 
> a largely scripted removal of unnecessary casts of the return value of 
> "kmalloc()", for example, or the switch to "ktermios" for the tty layer, 
> or the introduction of "struct path" in the VFS layer instead of keeping 
> the f_{dentry,vfsmnt} entries separate, or indeed the removal of SLAB_xxx 
> constant names in favour of the standard GFP_xxx ones).
> 
> So while the patch itself isn't actually all that much smaller than usual, 
> at least my personal gut feel is that the actual changes are not as 
> intrusive, just in some cases have big diffs.
> 
> But both the diffstat and the shortlog are still too big to fit in the 
> kernel mailing list limits, so you'll just have to take my word for it. Or 
> get the git repo, and do your own delving into things with
> 
> 	git log v2.6.19..v2.6.20-rc1 | git shortlog
> 
> There _are_ a few areas of note:
> 
>  - the aforementioned "workqueue" changes (where we still have some work 
>    to do to finalize the proper actions on all architectures: it's being 
>    somewhat discussed on the arch mailing lists, hopefully we'll have it 
>    all resolved by -rc2, and it doesn't really worry me)
> 
>  - lockless page cache (RCU lookups of radix trees)
> 
>  - kvm driver for all those crazy virtualization people to play with
> 
>  - networking updates (DCCP, address-family agnostic connection tracking 
>    in netfilter, sparse byte order annotations, yadda yadda)
> 
>  - HID layer separated out of the USB stuff (bluetooth apparently wants 
>    the HID stuff too)
> 
>  - tons and tons of driver (ftape removal, ATA, pcmcia, i2c, 
>    infiniband, dvb, networking..) and architecture updates (arm, mips, 
>    powerpc, sh)
> 
Did I miss an alternate method of handling ftape devices, or are these 
old beasts now unsupported? I occasionally have to be able to handle 
that media, since the industrial device using ftape for control updates 
cost more than a small house.

I can obviously keep an old slow machine to do the job, but I'd like to 
know if I need to.

-- 
bill davidsen <davidsen@tmr.com>
   CTO TMR Associates, Inc
   Doing interesting things with small computers since 1979

Re: Linux 2.6.20-rc1

[Alan]

On Fri, 15 Dec 2006 11:50:14 -0500
Bill Davidsen <davidsen@tmr.com> wrote:
> Did I miss an alternate method of handling ftape devices, or are these 
> old beasts now unsupported? I occasionally have to be able to handle 
> that media, since the industrial device using ftape for control updates 
> cost more than a small house.

Do you have hardware and the time to at least test cleanups ?

> I can obviously keep an old slow machine to do the job, but I'd like to 
> know if I need to.

The assumption was that since in 2.6 it was so ancient and unloved that
nobody had even seen an ftape device this century. If it is still being
used and you can test cleanups then the removal should be reverted

Alan

Re: Linux 2.6.20-rc1

[Bill Davidsen]

Alan wrote:
> On Fri, 15 Dec 2006 11:50:14 -0500
> Bill Davidsen <davidsen@tmr.com> wrote:
>   
>> Did I miss an alternate method of handling ftape devices, or are these 
>> old beasts now unsupported? I occasionally have to be able to handle 
>> that media, since the industrial device using ftape for control updates 
>> cost more than a small house.
>>     
>
> Do you have hardware and the time to at least test cleanups ?
>
>   
>> I can obviously keep an old slow machine to do the job, but I'd like to 
>> know if I need to.
>>     
>
> The assumption was that since in 2.6 it was so ancient and unloved that
> nobody had even seen an ftape device this century. If it is still being
> used and you can test cleanups then the removal should be reverted

As much as I have in the past supported keeping useful features in the 
kernel, this one can go from 2.6 as far as I'm concerned. I would hate 
to see anyone spend any time maintaining something which is so little 
used. I can easily move the hardware to a 2.4 machine, or something 
running an early 2.6.

I think "ancient and unloved" is an apt description.

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979