Bug 3660

Summary: Неправильная работа F_GETLK
Product: CIFS@Etersoft Reporter: Виталий Перов <vitperov>
Component: блокировки файлов и доступAssignee: Pavel Shilovsky <piastry>
Status: CLOSED FIXED QA Contact:
Severity: blocker    
Priority: P1 CC: kipruss, lav, lbeasty, piastry, sin
Version: не указана   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Whiteboard:
Заявки RT: Связано с:
Дата напоминания:
Bug Depends on:    
Bug Blocks: 1185    
Attachments: Архив с тестом

Description Виталий Перов 2009-03-16 12:14:32 MSK
Написал тест:
1) устанавливается блокировка на файл (на
запись)
2) с помощью fork порождается новый процесс
3) и в потомке, и в родителе проверяется
блокировка на файл

Результаты:

*Запуск на ext3:
Родитель - блокировка отсутствует
Потомок - стоит блокировка на запись

*Запуск на cifs:
Родитель - блокировка отсутствует
Потомок - стоит блокировка на ЧТЕНИЕ

Сам тест выложу чуть позже
Comment 1 Виталий Перов 2009-03-16 13:56:12 MSK
Опубликовал тест в wine-etersoft-devel/locks/test_for_cifs
Comment 2 Konstantin Baev 2009-03-16 14:47:47 MSK
(In reply to comment #1)
> Опубликовал тест в wine-etersoft-devel/locks/test_for_cifs
> 

Можно полную ссылку?
Comment 3 Виталий Перов 2009-03-16 14:59:42 MSK
> Можно полную ссылку?
> 

Это сvs-репозиторий. Клонируется коммандой:
cvs checkout wine-etersoft-devel
Comment 4 Konstantin Baev 2009-03-16 18:34:23 MSK
(In reply to comment #3)
> > Можно полную ссылку?
> > 
> 
> Это сvs-репозиторий. Клонируется коммандой:
> cvs checkout wine-etersoft-devel
> 

Пытался в склонировать:

[kipruss@server ~]$ cvs checkout wine-etersoft-devel
kipruss@office.etersoft.ru's password:
cvs checkout: cannot open directory /var/local/cvsroot/wine-etersoft-devel: Permission denied
cvs checkout: skipping directory wine-etersoft-devel
[kipruss@server ~]$ ssh design
kipruss@design's password:
Last login: Mon Mar 16 18:30:43 2009 from server.office.etersoft.ru
[kipruss@design ~]$ cvs checkout wine-etersoft-devel
cvs checkout: No CVSROOT specified!  Please use the `-d' option
cvs [checkout aborted]: or set the CVSROOT environment variable.
[kipruss@design ~]$ cvs checkout -d wine-etersoft-devel
cvs checkout: No CVSROOT specified!  Please use the `-d' option
cvs [checkout aborted]: or set the CVSROOT environment variable.

Может проще или указать git-репозиторий или просто файлы прислать?
Comment 5 Виталий Перов 2009-03-16 20:05:53 MSK
Created attachment 1106 [details]
Архив с тестом
Comment 6 Vitaly Lipatov 2009-03-19 16:01:11 MSK
(In reply to comment #4)
...
> [kipruss@server ~]$ cvs checkout wine-etersoft-devel
> kipruss@office.etersoft.ru's password:
> cvs checkout: cannot open directory /var/local/cvsroot/wine-etersoft-devel:
> Permission denied
...
> Может проще или указать git-репозиторий или
> просто файлы прислать?
Если что, я права сделал. 

Comment 7 Pavel Shilovsky 2009-03-25 23:36:34 MSK
1. Стоит блокировка на запись, выставленная другим процессом.
1)
lock.l_type = F_RDCLK;
fcntl(f, F_GETLK, &lock);
[piastry@tartarus test_for_cifs]$ ./a.out ~/upload/1
read
[piastry@tartarus test_for_cifs]$ ./a.out ~/1
write

2)
lock.l_type = F_WRCLK;
fcntl(f, F_GETLK, &lock);
[piastry@tartarus test_for_cifs]$ ./a.out ~/upload/1
write
[piastry@tartarus test_for_cifs]$ ./a.out ~/1
write


2. Стоит блокировка на чтение, выставленная другим процессом.
1)
lock.l_type = F_RDCLK;
fcntl(f, F_GETLK, &lock);
[piastry@tartarus test_for_cifs]$ ./a.out ~/1
no
[piastry@tartarus test_for_cifs]$ ./a.out ~/upload/1
no

2)
lock.l_type = F_WRCLK;
fcntl(f, F_GETLK, &lock);
[piastry@tartarus test_for_cifs]$ ./a.out ~/1
read
[piastry@tartarus test_for_cifs]$ ./a.out ~/upload/1
write

Параметры монтирования шары в папку "upload": noperm,user,rw.

Как можно заметить fork() тут не причём.

Comment 8 Евгений Синельников 2009-03-25 23:40:42 MSK
(In reply to comment #7)
> Как можно заметить fork() тут не причём.
> 

Тогда это явная бага cifs, верно?
Comment 9 Pavel Shilovsky 2009-03-26 00:33:15 MSK
Верно. Решил проблему. Проблема была именно в команде F_GETLK.
Результаты:

1. Стоит блокировка на запись, выставленная другим процессом.
1)
lock.l_type = F_RDCLK;
fcntl(f, F_GETLK, &lock);
[piastry@tartarus test_for_cifs]$ ./a.out ~/1
write
[piastry@tartarus test_for_cifs]$ ./a.out ~/upload/1
write

2)
lock.l_type = F_WRCLK;
fcntl(f, F_GETLK, &lock);
[piastry@tartarus test_for_cifs]$ ./a.out ~/1
write
[piastry@tartarus test_for_cifs]$ ./a.out ~/upload/1
write

2. Стоит блокировка на чтение, выставленная другим процессом.
1)
lock.l_type = F_RDCLK;
fcntl(f, F_GETLK, &lock);
[piastry@tartarus test_for_cifs]$ ./a.out ~/1
no
[piastry@tartarus test_for_cifs]$ ./a.out ~/upload/1
no

2)
lock.l_type = F_WRCLK;
fcntl(f, F_GETLK, &lock);
[piastry@tartarus test_for_cifs]$ ./a.out ~/1
read
[piastry@tartarus test_for_cifs]$ ./a.out ~/upload/1
read

Параметры монтирования шары в папку "upload": noperm,user,rw.

Поведение корректное.

Патч.
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=commitdiff;h=f558d1a40bc8008799c47a1373800912a9525860
Comment 10 Konstantin Baev 2009-03-26 00:58:25 MSK
(In reply to comment #9)
> Решил проблему.

> Патч.
> http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=commitdiff;h=f558d1a40bc8008799c47a1373800912a9525860
> 

Круто!

Недавно релизнулось ядро 2.6.29 и у меня были намерения когда будет время выпустить новый etercifs с этим ядром. Но, учитывая это решение, я полагаю, что это нужно сделать срочно. Плюс ко всему заменить патч к проблеме #3626 на более красивый.

Спрашиваю мнение общественности на этот счёт.
Comment 11 Vitaly Lipatov 2009-03-26 09:03:58 MSK
(In reply to comment #10)
...
> Недавно релизнулось ядро 2.6.29 и у меня были
> намерения когда будет время выпустить
> новый etercifs с этим ядром. Но, учитывая это
> решение, я полагаю, что это нужно сделать
> срочно. Плюс ко всему заменить патч к
> проблеме #3626 на более красивый.
Я думаю, 2.6.29 - по мере возможности. Исправление к этой баге и к 3626 было бы хорошо потестить.

Но без решения 1185 смысла в релизе нет.
Comment 12 Konstantin Baev 2009-03-26 13:24:27 MSK
(In reply to comment #11)
> (In reply to comment #10)
> ...
> > Недавно релизнулось ядро 2.6.29 и у меня были
> > намерения когда будет время выпустить
> > новый etercifs с этим ядром. Но, учитывая это
> > решение, я полагаю, что это нужно сделать
> > срочно. Плюс ко всему заменить патч к
> > проблеме #3626 на более красивый.
> Я думаю, 2.6.29 - по мере возможности.
> Исправление к этой баге и к 3626 было бы
> хорошо потестить.
> 
> Но без решения 1185 смысла в релизе нет.
> 

К баге 3626 исправление уже есть в текущей версии. Я имел в виду только лишь code refactoring. Стало быть я сейчас сделаю тестовую версию с Пашиным патчем только чтобы потестить и проверить решение в контексте проблемы из баги 1185.
Comment 13 Konstantin Baev 2009-03-26 16:01:36 MSK
Сделал тестовую сборку.

Пакет: ftp://ftp.etersoft.ru/pub/Etersoft/LINUX@Etersoft/Boxes/etercifs-testing/noarch/RPMS.default/etercifs-4.2.1-alt1.testing1.noarch.rpm

Гит (бранч testing2): http://git.etersoft.ru/people/kipruss/packages/?p=etercifs.git;a=shortlog;h=refs/heads/testing2

Результаты тестирования, думаю, надо писать уже в 1185.
Comment 14 Elena V. Gurevich 2009-03-27 17:22:21 MSK
Собрала новую версию тестов.(0.0.8-alt2). Добавлен тест в SetLock на getLock. 
И он показывает, что в новом етерцифсе ошибка не исправлена.

[lena@ximinez rect-tests]$ rpm -qa | grep cifs
etercifs-4.2.1-alt1.testing1

[lena@ximinez rect-tests]$ uname -r
2.6.27-std-def-alt11
Comment 15 Pavel Shilovsky 2009-03-27 17:56:54 MSK
Хотелось бы взглянуть на эту версию тестов. Тесты, которые я приводил выше проходят на новом etercifs. Это локальные тесты, написанные на си.
Comment 16 Pavel Shilovsky 2009-03-27 23:32:40 MSK
Разобрался. Проблема была в том, что в ректе монтировалось с параметром forcemand. А его мой патч не затрагивает.
Comment 17 Konstantin Baev 2009-03-27 23:59:28 MSK
(In reply to comment #16)
> Разобрался. Проблема была в том, что в ректе
> монтировалось с параметром forcemand. А его мой
> патч не затрагивает.
> 

Не совсем понятно - нужно дополнить патч, чтобы и с forcemand было нормально или с ним и так все хорошо?
Comment 18 Pavel Shilovsky 2009-03-28 14:03:17 MSK
Конечно нужно дополнить! Скоро будет патч.
Comment 19 Pavel Shilovsky 2009-03-28 14:10:16 MSK
Приготовил патч.

http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=commitdiff;h=9e56bf05cd6065f3e4e85511ddfb071b38375bc0

Можно тестировать.
Comment 20 Pavel Shilovsky 2009-03-28 14:14:34 MSK
На всякий случай - этот патч дополняет предыдущий. Для корректной работы с опцией forcemand и без неё нужны оба.
Comment 21 Pavel Shilovsky 2009-03-28 20:45:55 MSK
Немного подправил последний патч и переписал коммит. Теперь смотреть тут:
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=commitdiff;h=4823e75431543fa704fd651b525e2a6366164101
Comment 22 Pavel Shilovsky 2009-03-30 10:05:09 MSD
Я нашёл некоторые баги, потому одним диффом дело уже не ограничивается. Все изменения в той ветке. следует брать дифф от HEAD^^^^ до HEAD
Comment 23 Konstantin Baev 2009-03-30 15:29:23 MSD
Сделал тестовую сборку - 2.

Пакет:
ftp://ftp.etersoft.ru/pub/Etersoft/LINUX@Etersoft/Boxes/etercifs-testing/noarch/RPMS.default/etercifs-4.2.1-alt1.testing2.noarch.rpm
Comment 24 Elena V. Gurevich 2009-03-30 18:02:54 MSD
Что предыдущая сборка, что нынешняя.. Что с forcemand, что без него..
Тесты на getlock (не все)отваливаются..
Comment 25 Konstantin Baev 2009-03-30 18:46:44 MSD
(In reply to comment #24)
> Что предыдущая сборка, что нынешняя.. Что с
> forcemand, что без него..
> Тесты на getlock (не все)отваливаются..
> 

Поскольку у нас тут временно отвалился DNS у провайдера, Лена не успела написать сюда, что она ещё раз перепроверила и на самом деле все хорошо с новой версией. Нужные тесты проходят.
Comment 26 Konstantin Baev 2009-03-30 21:09:02 MSD
см. http://bugs.etersoft.ru/show_bug.cgi?id=3755#c1
выпущен 4.3.0, содержащий решение проблемы
Comment 27 Elena V. Gurevich 2009-03-31 12:41:53 MSD
Соврала. Последняя версия етерцифса работает на гетлок нормально на тестах.
Тесты проходят независимо от указания опции forcemand