Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 16 Sep 2019 20:14:17 +1000
From:   Matthew Bobrowski <mbobrowski@mbobrowski.org>
To:     Ritesh Harjani <riteshh@linux.ibm.com>
Cc:     tytso@mit.edu, jack@suse.cz, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        david@fromorbit.com, hch@infradead.org, darrick.wong@oracle.com
Subject: Re: [PATCH v3 5/6] ext4: introduce direct IO write path using iomap
 infrastructure
Message-ID: <20190916101417.GA4024@bobrowski>
References: <cover.1568282664.git.mbobrowski@mbobrowski.org>
 <db33705f9ba35ccbe20fc19b8ecbbf2078beff08.1568282664.git.mbobrowski@mbobrowski.org>
 <20190916043741.BCBDEA4054@b06wcsmtp001.portsmouth.uk.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190916043741.BCBDEA4054@b06wcsmtp001.portsmouth.uk.ibm.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

On Mon, Sep 16, 2019 at 10:07:41AM +0530, Ritesh Harjani wrote:
> > @@ -213,12 +214,16 @@ static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from)
> >   	struct inode *inode = file_inode(iocb->ki_filp);
> >   	ssize_t ret;
> > 
> > +	if (unlikely(IS_IMMUTABLE(inode)))
> > +		return -EPERM;
> > +
> >   	ret = generic_write_checks(iocb, from);
> >   	if (ret <= 0)
> >   		return ret;
> > 
> > -	if (unlikely(IS_IMMUTABLE(inode)))
> > -		return -EPERM;
> > +	ret = file_modified(iocb->ki_filp);
> > +	if (ret)
> > +		return 0;
> 
> Why not return ret directly, otherwise we will be returning the wrong
> error code to user space. Thoughts?

You're right. I can't remember exactly why I decided to return '0', however
looking at the code once again I don't see a reason why we don't just return
'ret', as any value other than '0' represents a failure in this case
anyway. Thanks for picking that up.
 
> Do you think simplification/restructuring of this API
> "ext4_write_checks" can be a separate patch, so that this patch
> only focuses on conversion of DIO write path to iomap?

Hm, if we split it up so that it comes before this patch then it becomes hairy
in the sense that a whole bunch of other changes would also need to come with
what looks to be such a miniscule modification
i.e. ext4_buffered_write_iter(), ext4_file_write_iter(), etc. Splitting it to
come after just doesn't make any sense. To be honest, I don't really have any
strong opinions around why we shouldn't split it up, nor do I have a strong
opinion around why we should, so I think we should just leave it for now.

> Also, I think we can make the function (ext4_write_checks())
> like below. This way we call for file_modified() only after we
> have checked for write limits, at one place.

No objections and I think it's a good idea.

>   static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter
> *from)
>   {
>           struct inode *inode = file_inode(iocb->ki_filp);
>           ssize_t ret;
> 
>           if (unlikely(IS_IMMUTABLE(inode)))
>                   return -EPERM;
> 
>           ret = generic_write_checks(iocb, from);
>           if (ret <= 0)
> _                 return ret;
>           /*
>            * If we have encountered a bitmap-format file, the size limit
>            * is smaller than s_maxbytes, which is for extent-mapped files.
>            */
>           if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
>                   struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> 
>                   if (iocb->ki_pos >= sbi->s_bitmap_maxbytes)
>                           return -EFBIG;
>                   iov_iter_truncate(from, sbi->s_bitmap_maxbytes -
> iocb->ki_pos);
>           }
> +
> +         ret = file_modified(iocb->ki_filp);
> +         if (ret)
> +                 return ret;
> +
>           return iov_iter_count(from);
>   }

--<M>--