----- Original Message -----
> From: "Bob Peterson" <rpete...@redhat.com>
> To: "Abhi Das" <a...@redhat.com>
> Cc: cluster-devel@redhat.com
> Sent: Monday, June 1, 2015 10:25:45 AM
> Subject: Re: [Cluster-devel] [GFS2 PATCH 1/2] gfs2: fix quota updates on
> block boundaries
>
> Hi,
>
> Some comments embedded.
>
> ----- Original Message -----
> >
> > +static int gfs2_write_buf_to_page(struct gfs2_inode *ip, unsigned long
> > index,
> > + unsigned off, void *buf, unsigned bytes)
> > +{
> > + struct inode *inode = &ip->i_inode;
> > + struct gfs2_sbd *sdp = GFS2_SB(inode);
> > + struct address_space *mapping = inode->i_mapping;
> > + struct page *page;
> > + struct buffer_head *bh;
> > + void *kaddr;
> > + unsigned long fs_blk;
>
> If this is truly a file system block, it should probably be u64, not
> unsigned long.
>
Yes. I'll fix this in my next version. That field is named poorly. It is the fs
block
offset of the page index supplied. For the quota file, unsigned long should be
plenty,
but my aim was to split this function out so it can be of general use. I'll fix
it to
u64.
> > + unsigned bsize = sdp->sd_sb.sb_bsize, bnum = 0, boff = 0;
> > + unsigned to_write = bytes, pg_off = off;
> > + int done = 0;
> > +
> > + fs_blk = index << (PAGE_CACHE_SHIFT - sdp->sd_sb.sb_bsize_shift);
> > + boff = off % bsize;
> > +
> > + page = find_or_create_page(mapping, index, GFP_NOFS);
> > + if (!page)
> > + return -ENOMEM;
> > + if (!page_has_buffers(page))
> > + create_empty_buffers(page, bsize, 0);
> > +
> > + bh = page_buffers(page);
> > + while (!done) {
> > + /* Find the beginning block within the page */
> > + if (pg_off >= ((bnum * bsize) + bsize)) {
> > + bh = bh->b_this_page;
> > + bnum++;
> > + fs_blk++;
> > + continue;
> > + }
> > + if (!buffer_mapped(bh)) {
> > + gfs2_block_map(inode, fs_blk, bh, 1);
> > + if (!buffer_mapped(bh))
> > + goto unlock_out;
> > + /* If it's a newly allocated disk block, zero it */
> > + if (buffer_new(bh))
> > + zero_user(page, bnum * bsize, bh->b_size);
> > + }
> > + if (PageUptodate(page))
> > + set_buffer_uptodate(bh);
> > + if (!buffer_uptodate(bh)) {
> > + ll_rw_block(READ | REQ_META, 1, &bh);
>
> I know this was just copied from the section below, but I don't think this
> read
> should have REQ_META set because it's not metadata, right? At any rate, it's
> treated as data in the trans_add_data below.
>
Yeah, another issue due to copying. I wonder if this piece was written before
REQ_PRIO
was introduced and was meant to issue a priority read or if we want to treat
the system
quota file contents as metadata.
Cheers!
--Abhi
