Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacingrq_for_each_bio

[Geert Uytterhoeven]

On Tue, 21 Aug 2007, Satyam Sharma wrote:
> 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¡Ç

You will get a warning, so you will know.

> 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. ]

[ and you dropped the cell mailing list from the CC list ]

> * 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):

Adding such a cast makes it impossible for the compiler to detect (and warn us)
if something has changed.

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

NAK. Do not add casts that are not needed. Casts are evil.

> 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);

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village  Da Vincilaan 7-D1  B-1935 Zaventem  Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@xxxxxxxxxxx	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7  B-1840 Londerzeel  Belgium	
VAT BE 0413.825.160 Â RPR Brussels	
Fortis Bank Zaventem  Swift GEBABEBB08A  IBAN BE39001382358619