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

[Matthew Wilcox]

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.

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

> > +		}
> > +	} 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 ... ?
--
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/