Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacingrq_for_each_bio

[Satyam Sharma]

Hi Geert,


On Mon, 20 Aug 2007, Geert Uytterhoeven wrote:

> On Sat, 18 Aug 2007, Satyam Sharma wrote:
> > On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > > >> [...]
> > > >> >  		dev_dbg(&dev->sbd.core,
> > > >> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
                                                                        ^^^

> > > >> > -			__func__, __LINE__, i, bio_segments(bio),
> > > >> > -			bio_sectors(bio), sector);
> > > >> > -		bio_for_each_segment(bvec, bio, j) {
> > > >> > +			__func__, __LINE__, i, bio_segments(iter.bio),
> > > >> > +			bio_sectors(iter.bio),
> > > >> > +			(unsigned long)iter.bio->bi_sector);
                                ^^^^^^^^^^^^^^^          ^^^^^^^^^

> > > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> > > 
> > > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > > so since sector_t may just change its shape underneath.
> > 
> > Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> > I was mentioning why the cast itself should be (unsigned long long) otoh.
> 
> On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
> on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
> no compiler warning to be silenced by adding the cast.

This is turning rather funny :-)

* Why the printk() conversion specifier must be "%llu"?

When reusing parts of this code (such as this debug message) for another
32-bit driver (we certainly seem to care about this, as per your reply),
the largest size of sector_t can be "unsigned long long", thereby causing
truncated output, and the following warning:

warning: format ‘%lu’ expects type ‘long unsigned int’, but argument 2 has
type ‘sector_t’

Therefore, let us not depend on the fact that this driver is being used
only for 64-bit platforms as of now (especially since this is in drivers/
and not in arch/) and rather make the printk() specifier as "%llu".

[ Sadly, I had to repeat most of my previous mail, which Jan Engelhardt
appears to have snipped. ]

* Why we require an explicit (unsigned long long) cast?

Having made the above change (and say we don't have an explicit cast
there), that line now becomes:

	printk("... %llu\n", ..., iter.bio->bi_sector);

Now if we build this code with CONFIG_LBD=n, sector_t becomes just an
"unsigned long" (whereas printk specifier is the larger "%llu") thereby
causing:

warning: format ‘%llu’ expects type ‘long long unsigned int’, but argument
2 has type ‘sector_t’

Therefore, let us shut this up with an explicit (unsigned long long) cast,
as is done in most other existing code in the kernel where we want to get
bi_sector printed out, which would correctly convert the value to the
larger integer type (even negative ones, due to sign-extension) and print
it out.


> On the other hand, adding a cast may hide bugs:
>   - cast to unsigned long long: When sector_t is changed to an even larger
>     type, it will be silently truncated as well.

IMHO, this is not a valid enough reason, given the above (BTW if/when that
happens, we'll have to update the printk conversion specifer as well).

Personally, I'd say code that assumes "sizeof(unsigned long long) ==
sizeof(unsigned long)", and also that assumes "sizeof(unsigned long) ==
sizeof(sector_t)" -- both assumptions _are_ made by the above code --
is not good taste, even if both may be true for PS3 platform. So unless
we decide "nobody except that platform would ever build this driver
anyway", I'd suggest to make the printk specifier as "%llu" and use an
explicit (unsigned long long) cast. Therefore (on top of this series):

Signed-off-by: Satyam Sharma <satyam@xxxxxxxxxxxxx>

---

diff -ruNp a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
--- a/drivers/block/ps3disk.c	2007-08-21 06:52:34.000000000 +0530
+++ b/drivers/block/ps3disk.c	2007-08-21 06:56:07.000000000 +0530
@@ -100,10 +100,10 @@ static void ps3disk_scatter_gather(struc
 	rq_for_each_segment(bvec, req, iter) {
 		unsigned long flags;
 		dev_dbg(&dev->sbd.core,
-			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
+			"%s:%u: bio %u: %u segs %u sectors from %llu\n",
 			__func__, __LINE__, i, bio_segments(iter.bio),
 			bio_sectors(iter.bio),
-			(unsigned long)iter.bio->bi_sector);
+			(unsigned long long)iter.bio->bi_sector);
 
 		size = bvec->bv_len;
 		buf = bvec_kmap_irq(bvec, flags);
@@ -142,8 +142,9 @@ static int ps3disk_submit_request_sg(str
 
 	start_sector = req->sector * priv->blocking_factor;
 	sectors = req->nr_sectors * priv->blocking_factor;
-	dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
-		__func__, __LINE__, op, sectors, start_sector);
+	dev_dbg(&dev->sbd.core, "%s:%u: %s %llu sectors starting at %llu\n",
+		__func__, __LINE__, op, (unsigned long long)sectors,
+		(unsigned long long)start_sector);
 
 	if (write) {
 		ps3disk_scatter_gather(dev, req, 1);