Re: [PATCH v11 08/21] dax,ext2: Replace ext2_clear_xip_target with dax_clear_blocks

[Mathieu Desnoyers]

----- Original Message -----
> From: "Matthew Wilcox" <willy@xxxxxxxxxxxxxxx>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: "Matthew Wilcox" <matthew.r.wilcox@xxxxxxxxx>, linux-fsdevel@xxxxxxxxxxxxxxx, linux-mm@xxxxxxxxx,
> linux-kernel@xxxxxxxxxxxxxxx
> Sent: Thursday, October 16, 2014 11:22:34 PM
> Subject: Re: [PATCH v11 08/21] dax,ext2: Replace ext2_clear_xip_target with dax_clear_blocks
> 
> On Thu, Oct 16, 2014 at 12:05:25PM +0200, Mathieu Desnoyers wrote:
> > > +int dax_clear_blocks(struct inode *inode, sector_t block, long size)
> > > +{
> > > +	struct block_device *bdev = inode->i_sb->s_bdev;
> > > +	sector_t sector = block << (inode->i_blkbits - 9);
> > 
> > Is there a define e.g. SECTOR_SHIFT rather than using this hardcoded "9"
> > value ?
> 
> Yeah ... in half a dozen drivers, so introducing them globally spews
> warnings about redefining macros.  The '9' and '512' are sprinkled all
> over the storage parts of the kernel, it's a complete flustercluck that
> I wasn't about to try to unscrew.

Fair enough.

> 
> > > +		while (count > 0) {
> > > +			unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
> > 
> > unsigned -> unsigned int
> 
> Any particular reason?  Omitting it in some places helps stay within
> the 80-column limit without sacrificing readability.

It looks like FS code often uses "unsigned", so I'm not too concerned.

It's just that I'm used to the Linux core kernel style, which tend to
use "unsigned int".

> 
> > > +		}
> > > +	} while (size);
> > 
> > Just to stay on the safe side, can we do while (size > 0) ? Just in case
> > an unforeseen issue makes size negative, and gets us in a very long loop.
> 
> If size < 0, we should BUG, because that means we've zeroed more than
> we were asked to do, which is data corruption.
> 
> There's probably some other hardening we should do for this loop.
> For example, if 'count' is < 512, it can go into an infinite loop.
> 
>         do {
>                 void *addr;
>                 unsigned long pfn;
>                 long count;
> 
>                 count = bdev_direct_access(bdev, sector, &addr, &pfn, size);
>                 if (count < 0)
>                         return count;
>                 while (count > 0) {
>                         unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
>                         if (pgsz > count)
>                                 pgsz = count;
>                         if (pgsz < PAGE_SIZE)
>                                 memset(addr, 0, pgsz);
>                         else
>                                 clear_page(addr);
>                         addr += pgsz;
>                         size -= pgsz;
>                         count -= pgsz;
> 			BUG_ON(pgsz & 511);
>                         sector += pgsz / 512;
>                         cond_resched();
>                 }
> 		BUG_ON(size < 0);
>         } while (size);
> 
> I think that should do the job ... ?
> 

Yep. I love defensive programming, especially for filesystems. :)

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/