[PATCH] ubifs: add missing ui pointer in debugging code

[PATCH] ubifs: add missing ui pointer in debugging code

[Daniel Golle]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

If UBIFS_DEBUG is defined an additional assertion of the ui_lock
spinlock in do_writepage cannot compile because the ui pointer has not
been previously declared.

Fix this by declaring and initializing the ui pointer in case
UBIFS_DEBUG is defined.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 fs/ubifs/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 4f34dba..e13ec5e 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -903,6 +903,7 @@ static int do_writepage(struct page *page, int len)
 	struct ubifs_info *c = inode->i_sb->s_fs_info;
 
 #ifdef UBIFS_DEBUG
+	struct ubifs_inode *ui = ubifs_inode(inode);
 	spin_lock(&ui->ui_lock);
 	ubifs_assert(page->index <= ui->synced_i_size << PAGE_CACHE_SIZE);
 	spin_unlock(&ui->ui_lock);
-- 
1.9.2


[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] ubifs: add missing ui pointer in debugging code

[hujianyang]

On 2014/5/14 4:27, Daniel Golle wrote:
> If UBIFS_DEBUG is defined an additional assertion of the ui_lock
> spinlock in do_writepage cannot compile because the ui pointer has not
> been previously declared.
> 
> Fix this by declaring and initializing the ui pointer in case
> UBIFS_DEBUG is defined.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  fs/ubifs/file.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 4f34dba..e13ec5e 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -903,6 +903,7 @@ static int do_writepage(struct page *page, int len)
>  	struct ubifs_info *c = inode->i_sb->s_fs_info;
>  
>  #ifdef UBIFS_DEBUG
> +	struct ubifs_inode *ui = ubifs_inode(inode);
>  	spin_lock(&ui->ui_lock);
>  	ubifs_assert(page->index <= ui->synced_i_size << PAGE_CACHE_SIZE);

I think here should be:
  	ubifs_assert(page->index <= ui->synced_i_size >> PAGE_CACHE_SHIFT);

as line 1023 in ubifs_writepage:

        /* Is the page fully inside @i_size? */
        if (page->index < end_index) {
                if (page->index >= synced_i_size >> PAGE_CACHE_SHIFT) {
                        err = inode->i_sb->s_op->write_inode(inode, NULL);

>  	spin_unlock(&ui->ui_lock);
> 

Re: [PATCH] ubifs: add missing ui pointer in debugging code

[Daniel]

On 05/14/2014 09:32 AM, hujianyang wrote:
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index 4f34dba..e13ec5e 100644
>> --- a/fs/ubifs/file.c
>> +++ b/fs/ubifs/file.c
>> @@ -903,6 +903,7 @@ static int do_writepage(struct page *page, int len)
>>  	struct ubifs_info *c = inode->i_sb->s_fs_info;
>>  
>>  #ifdef UBIFS_DEBUG
>> +	struct ubifs_inode *ui = ubifs_inode(inode);
>>  	spin_lock(&ui->ui_lock);
>>  	ubifs_assert(page->index <= ui->synced_i_size << PAGE_CACHE_SIZE);
> 
> I think here should be:
>   	ubifs_assert(page->index <= ui->synced_i_size >> PAGE_CACHE_SHIFT);
> 
> as line 1023 in ubifs_writepage:
> 
>         /* Is the page fully inside @i_size? */
>         if (page->index < end_index) {
>                 if (page->index >= synced_i_size >> PAGE_CACHE_SHIFT) {
>                         err = inode->i_sb->s_op->write_inode(inode, NULL);
> 

My intention was merely to get the code compiled with UBIFS_DEBUG enabled. If
things further than that need to be sorted out in that place as well, I suggest
that should go in a separate discussion.


Cheers

Daniel

Re: [PATCH] ubifs: add missing ui pointer in debugging code

[Artem Bityutskiy]

On Tue, 2014-05-13 at 22:27 +0200, Daniel Golle wrote:
> If UBIFS_DEBUG is defined an additional assertion of the ui_lock
> spinlock in do_writepage cannot compile because the ui pointer has not
> been previously declared.
> 
> Fix this by declaring and initializing the ui pointer in case
> UBIFS_DEBUG is defined.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Pushed to linux-ubifs.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] ubifs: add missing ui pointer in debugging code

[Artem Bityutskiy]

On Wed, 2014-05-14 at 15:32 +0800, hujianyang wrote:
> On 2014/5/14 4:27, Daniel Golle wrote:
> > If UBIFS_DEBUG is defined an additional assertion of the ui_lock
> > spinlock in do_writepage cannot compile because the ui pointer has not
> > been previously declared.
> > 
> > Fix this by declaring and initializing the ui pointer in case
> > UBIFS_DEBUG is defined.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  fs/ubifs/file.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> > index 4f34dba..e13ec5e 100644
> > --- a/fs/ubifs/file.c
> > +++ b/fs/ubifs/file.c
> > @@ -903,6 +903,7 @@ static int do_writepage(struct page *page, int len)
> >  	struct ubifs_info *c = inode->i_sb->s_fs_info;
> >  
> >  #ifdef UBIFS_DEBUG
> > +	struct ubifs_inode *ui = ubifs_inode(inode);
> >  	spin_lock(&ui->ui_lock);
> >  	ubifs_assert(page->index <= ui->synced_i_size << PAGE_CACHE_SIZE);
> 
> I think here should be:
>   	ubifs_assert(page->index <= ui->synced_i_size >> PAGE_CACHE_SHIFT);

I think you are right, thanks. I've just applied this patch:

Author: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date:   Tue May 27 15:24:39 2014 +0300

    UBIFS: fix debugging check
    
    The debugging check which verifies that we never write outside of the file
    length was incorrect, since it was multiplying file length by the page size,
    instead of dividing. Fix this.
    
    Spotted-by: hujianyang <hujianyang@huawei.com>
    Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 727506b..2767ffb 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -905,7 +905,7 @@ static int do_writepage(struct page *page, int len)
 #ifdef UBIFS_DEBUG
        struct ubifs_inode *ui = ubifs_inode(inode);
        spin_lock(&ui->ui_lock);
-       ubifs_assert(page->index <= ui->synced_i_size << PAGE_CACHE_SIZE);
+       ubifs_assert(page->index <= ui->synced_i_size >> PAGE_CACHE_SIZE);
        spin_unlock(&ui->ui_lock);
 #endif

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] ubifs: add missing ui pointer in debugging code

[hujianyang]

Hi Artem,

I think you made a small mistake:

>         spin_lock(&ui->ui_lock);
> -       ubifs_assert(page->index <= ui->synced_i_size << PAGE_CACHE_SIZE);
> +       ubifs_assert(page->index <= ui->synced_i_size >> PAGE_CACHE_SIZE);

Here should be PAGE_CACHE_SHIFT, not PAGE_CACHE_SIZE.

Thanks.

Hu

Re: [PATCH] ubifs: add missing ui pointer in debugging code

[Artem Bityutskiy]

On Wed, 2014-05-28 at 10:27 +0800, hujianyang wrote:
> Hi Artem,
> 
> I think you made a small mistake:

Indeed, and not that small, even. I'll fix this, thanks!

If I was writing this code now, I would not bother using shifts, I'd
just use divisions, which are simpler and less error-prone. And modern
compilers are smart enough to optimize the code to use shifts when the
divider is a constant.

-- 
Best Regards,
Artem Bityutskiy